Skip to content

Hashtable: only shrink in Put and fix various other issues #998

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
charlievieth wants to merge 1 commit into
base: main
Choose a base branch
from
+98 −39
Open

Hashtable: only shrink in Put and fix various other issues #998

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 94 additions & 36 deletions Hashtable.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ in the source distribution for its full text.
#include "Hashtable.h"

#include <assert.h>
#include <inttypes.h>
#include <stddef.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
Expand Down Expand Up @@ -88,32 +90,91 @@ size_t Hashtable_count(const Hashtable* this) {

#endif /* NDEBUG */

/* https://oeis.org/A014234 */
static const uint64_t OEISprimes[] = {
7, 13, 31, 61, 127, 251, 509, 1021, 2039, 4093, 8191,
16381, 32749, 65521, 131071, 262139, 524287, 1048573,
2097143, 4194301, 8388593, 16777213, 33554393,
67108859, 134217689, 268435399, 536870909, 1073741789,
2147483647, 4294967291, 8589934583, 17179869143,
34359738337, 68719476731, 137438953447
#define MIN_TABLE_SIZE 11

/* Primes borrowed from gnulib/lib/gl_anyhash_primes.h.

Array of primes, approximately in steps of factor 1.2.
This table was computed by executing the Common Lisp expression
(dotimes (i 244) (format t "nextprime(~D)~%" (ceiling (expt 1.2d0 i))))
and feeding the result to PARI/gp. */
static const size_t primes[] = {
MIN_TABLE_SIZE, 13, 17, 19, 23, 29, 37, 41, 47, 59, 67, 83, 97, 127, 139,
167, 199, 239, 293, 347, 419, 499, 593, 709, 853, 1021, 1229, 1471, 1777,
2129, 2543, 3049, 3659, 4391, 5273, 6323, 7589, 9103, 10937, 13109, 15727,
18899, 22651, 27179, 32609, 39133, 46957, 56359, 67619, 81157, 97369,
116849, 140221, 168253, 201907, 242309, 290761, 348889, 418667, 502409,
602887, 723467, 868151, 1041779, 1250141, 1500181, 1800191, 2160233,
2592277, 3110741, 3732887, 4479463, 5375371, 6450413, 7740517, 9288589,
11146307, 13375573, 16050689, 19260817, 23112977, 27735583, 33282701,
39939233, 47927081, 57512503, 69014987, 82818011, 99381577, 119257891,
143109469, 171731387, 206077643, 247293161, 296751781, 356102141, 427322587,
512787097, 615344489, 738413383, 886096061, 1063315271, 1275978331,
1531174013, 1837408799, 2204890543UL, 2645868653UL, 3175042391UL,
3810050851UL,
/* on 32-bit make sure we do not return primes not fitting in size_t */
#if SIZE_MAX > 4294967295ULL
4572061027ULL, 5486473229ULL, 6583767889ULL, 7900521449ULL, 9480625733ULL,
/* Largest possible size should be 13652101063ULL == GROWTH_RATE((UINT32_MAX/3)*4)
we include some larger values in case the above math is wrong */
11376750877ULL, 13652101063ULL, 16382521261ULL, 19659025513ULL, 23590830631ULL,
#endif
};
Comment on lines +93 to 122
Copy link
Copy Markdown
Member

@BenBE BenBE May 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this list should stay much sparser as it was before (maybe not as sparse, but not this dense either).

The reason the OEIS series was chosen in #277 was because primes just below a power of two seemed to empirically give the fewest collisions. Also the resize of a hash table is quite expensive and thus should occur as rarely as possible.

Thus I'd much prefer to keep the scaling factor in that list of primes a bit larger at >=~1.5. With this new list at scaling factor 1.2 we are resizing the hash table on common systems about 30 times before we get into a steady state (for some desktop system @ ~3000 threads). With a scaling factor at around 1.5 this number would be roughly cut in half.

Copy link
Copy Markdown
Contributor

@tanriol tanriol May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simulating the logic of this PR suggests that, with the logic defined here, growth usually happens 4 steps at a time (3 or 5 at some specific initial sizes), while shrinking usually happens 3 steps at a time or more due to deferred shrinking (4 steps or more at some initial sizes).
Using the logic in this PR with the OEIS list would result in some sizes increasing by 2 steps at once and in some cases in shouldResize firing while the shrunk size is still the same, which would be suboptimal but would also work.
In my opinion, 1.2 is too low for the hash size ratio, but I don't really understand the motivation for this part of this PR. Why do we want to have more size steps when the load ratio and expand/shrink ratio stay the same?

Copy link
Copy Markdown
Member

@BenBE BenBE May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still open and should be resolved.


static size_t nextPrime(size_t n) {
/* on 32-bit make sure we do not return primes not fitting in size_t */
for (size_t i = 0; i < ARRAYSIZE(OEISprimes) && OEISprimes[i] < SIZE_MAX; i++) {
if (n <= OEISprimes[i])
return OEISprimes[i];
for (size_t i = 0; i < ARRAYSIZE(primes); i++) {
if (n < primes[i]) {
return primes[i];
}
}

CRT_fatalError("Hashtable: no prime found");
}

Hashtable* Hashtable_new(size_t size, bool owner) {
Hashtable* this;
/* USABLE_FRACTION is the maximum hash map load.
* Currently set to 2/3 capacity.
*
* Testing indicates that the median and average probe length
* increases significantly after 2/3 of the hash map capacity.
*
* load = {size,capacity} / items
*
* | load | probe max | probe avg | probe median |
* | ---- | --------- | --------- | ------------ |
* | 0.60 | 90.58 | 20.19 | 0.65 |
* | 0.65 | 167.00 | 37.07 | 1.58 |
* | 0.70 | 230.54 | 61.40 | 15.54 |
* | 0.75 | 287.00 | 85.23 | 26.15 |
* | 0.80 | 287.00 | 94.71 | 55.93 |
*/
#define USABLE_FRACTION(n) (((n) << 1)/3)
Copy link
Copy Markdown
Member

@BenBE BenBE May 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer normal multiplication.

Suggested change
#define USABLE_FRACTION(n) (((n) << 1)/3)
#define USABLE_FRACTION(n) (((n) * 2)/3)

Copy link
Copy Markdown
Member

@BenBE BenBE May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point that should be integrated into this patch series …


/* SHRINK_THRESHOLD is number of items at with the hash map should shrink.
* Currently set to 1/4 of the USABLE_FRACTION, which is ~13% of the total
* hash map size.
*/
#define SHRINK_THRESHOLD(n) (USABLE_FRACTION((n)) / 4)
Copy link
Copy Markdown
Member

@BenBE BenBE May 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary parens

Suggested change
#define SHRINK_THRESHOLD(n) (USABLE_FRACTION((n)) / 4)
#define SHRINK_THRESHOLD(n) (USABLE_FRACTION(n) / 4)

Copy link
Copy Markdown
Member

@BenBE BenBE May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further code style stuff that is not resolved …


/* GROWTH_RATE. Growth rate upon hitting maximum load.
* Currently set to items*3.
* This means that hashes double in size when growing without deletions,
* but have more head room when the number of deletions is on a par with the
* number of insertions.
*/
#define GROWTH_RATE(h) ((h)->items*3)

static inline bool Hashtable_shouldResize(const Hashtable* this) {
/* grow table */
return this->items >= USABLE_FRACTION(this->size) ||
/* shrink table */
(this->size > MIN_TABLE_SIZE && this->items <= SHRINK_THRESHOLD(this->size));
}

this = xMalloc(sizeof(Hashtable));
Hashtable* Hashtable_new(size_t size, bool owner) {
assert(MIN_TABLE_SIZE == primes[0]);
Hashtable* this = xMalloc(sizeof(Hashtable));
this->size = nextPrime(size);
this->items = 0;
this->size = size ? nextPrime(size) : 13;
this->buckets = (HashtableItem*) xCalloc(this->size, sizeof(HashtableItem));
this->owner = owner;

Expand Down Expand Up @@ -141,6 +202,10 @@ void Hashtable_clear(Hashtable* this) {
assert(Hashtable_isConsistent(this));
}

static inline size_t inc_index(size_t index, size_t size) {
return ++index != size ? index : 0;
Comment thread
BenBE marked this conversation as resolved.
}

static void insert(Hashtable* this, ht_key_t key, void* value) {
size_t index = key % this->size;
size_t probe = 0;
Expand Down Expand Up @@ -177,7 +242,7 @@ static void insert(Hashtable* this, ht_key_t key, void* value) {
value = tmp.value;
}

index = (index + 1) % this->size;
index = inc_index(index, this->size);
probe++;

assert(index != origIndex);
Expand All @@ -188,13 +253,13 @@ void Hashtable_setSize(Hashtable* this, size_t size) {

assert(Hashtable_isConsistent(this));

if (size <= this->items)
return;

/* newSize will always be >= MIN_TABLE_SIZE */
size_t newSize = nextPrime(size);
if (newSize == this->size)
return;

assert(newSize > this->items);
Comment thread
charlievieth marked this conversation as resolved.

HashtableItem* oldBuckets = this->buckets;
size_t oldSize = this->size;

Expand All @@ -221,12 +286,9 @@ void Hashtable_put(Hashtable* this, ht_key_t key, void* value) {
assert(this->size > 0);
assert(value);

/* grow on load-factor > 0.7 */
if (10 * this->items > 7 * this->size) {
if (SIZE_MAX / 2 < this->size)
CRT_fatalError("Hashtable: size overflow");

Hashtable_setSize(this, 2 * this->size);
/* Resize the hash table, if necessary, before inserting */
if (Hashtable_shouldResize(this)) {
Hashtable_setSize(this, GROWTH_RATE(this));
}

Comment thread
charlievieth marked this conversation as resolved.
insert(this, key, value);
Expand Down Expand Up @@ -255,14 +317,14 @@ void* Hashtable_remove(Hashtable* this, ht_key_t key) {
res = this->buckets[index].value;
}

size_t next = (index + 1) % this->size;
size_t next = inc_index(index, this->size);

while (this->buckets[next].value && this->buckets[next].probe > 0) {
this->buckets[index] = this->buckets[next];
this->buckets[index].probe -= 1;

index = next;
next = (index + 1) % this->size;
next = inc_index(index, this->size);
}

/* set empty after backward shifting */
Expand All @@ -275,7 +337,7 @@ void* Hashtable_remove(Hashtable* this, ht_key_t key) {
if (this->buckets[index].probe < probe)
break;

index = (index + 1) % this->size;
index = inc_index(index, this->size);
probe++;

assert(index != origIndex);
Expand All @@ -284,14 +346,10 @@ void* Hashtable_remove(Hashtable* this, ht_key_t key) {
assert(Hashtable_isConsistent(this));
assert(Hashtable_get(this, key) == NULL);

/* shrink on load-factor < 0.125 */
if (8 * this->items < this->size)
Hashtable_setSize(this, this->size / 3); /* account for nextPrime rounding up */

return res;
}

void* Hashtable_get(Hashtable* this, ht_key_t key) {
void* Hashtable_get(const Hashtable* this, ht_key_t key) {
size_t index = key % this->size;
size_t probe = 0;
void* res = NULL;
Expand All @@ -310,7 +368,7 @@ void* Hashtable_get(Hashtable* this, ht_key_t key) {
if (this->buckets[index].probe < probe)
break;

index = (index + 1) != this->size ? (index + 1) : 0;
index = inc_index(index, this->size);
probe++;

assert(index != origIndex);
Expand All @@ -319,7 +377,7 @@ void* Hashtable_get(Hashtable* this, ht_key_t key) {
return res;
}

void Hashtable_foreach(Hashtable* this, Hashtable_PairFunction f, void* userData) {
void Hashtable_foreach(const Hashtable* this, Hashtable_PairFunction f, void* userData) {
assert(Hashtable_isConsistent(this));
for (size_t i = 0; i < this->size; i++) {
HashtableItem* walk = &this->buckets[i];
Expand Down
7 changes: 4 additions & 3 deletions Hashtable.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ in the source distribution for its full text.

#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>


typedef unsigned int ht_key_t;
Copy link
Copy Markdown
Contributor

@tanriol tanriol May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure you can do that? Note that Hashtable is used in quite a few places in the codebase, and at least one uses it to store an inode number, which can be 64-bit at least on 64-bit systems (on 32-bit ones that seems to depend on _FILE_OFFSET_BITS=64).

Copy link
Copy Markdown
Member

@BenBE BenBE Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. unsigned int had been dodgy and uint32_t is just plain wrong. Good catch.

If we want to replace this, then uint64_t is the way to go, independent of the system architecture.

Also, the overall key handling for the hash table is a bit shaky to say the least: In C++ the STL uses a two-stage approach with the hash function for indexing, but a separate equals check on the full key object as a second layer in case the internal hash collided. Our implementation lacks this second comparison, thus we have to guarantee both in the key space as well as the hashing for reduction (which we currently don't do anywhere AFAIK) that no collissions occure (which can be reasonably assured, when we don't do hashing for key space reduction* and do only use unique keys within the (reduced) keyspace)

*Casting uint64_t to uint32_t is a hash function just masking to keep the lower bits.

Copy link
Copy Markdown
Contributor

@tanriol tanriol Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hashtables are used with the following keys in the current htop code: PIDs; user IDs; BSD jail IDs; inode numbers (in LinuxProcessList_readMaps); PCP column and meter IDs.

On my system (Linux amd64) pid_t and uid_t are defined as (signed) int, and PCP IDs are counted sequentially, so these shall never have any problems with this. I don't know about BSD jail IDs.

Inode numbers, on the other hand, on Linux boil down to either unsigned 32-bit (by default on 32-bit) or unsigned 64-bit (on 64-bit or with _FILE_OFFSET_BITS=64), so this is the only case in the current codebase that can cause confusion. Fortunately, it's only used in LinuxProcessList_readMaps and thus the only possible problem (on Linux) seems to be usesDeletedLib and/or libraries size being wrong on 32-bit systems.

P.S. And if one really wants to reduce hashtable allocations, I'd suggest starting with making library size calc use some persistent hashtable.

Copy link
Copy Markdown
Member

@BenBE BenBE Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thus if I get your list correctly, we mostly have unsigned values anyway (except for pid_t and uid_t).

Maybe the best (and cleanest) way forward is to make the hash functions for each type of key we use explicit:

static inline hkey_t Hashtable_keyFromUID(uit_t value) {
   return (hkey_t)value;
}
static inline hkey_t Hashtable_keyFromInode(inode_t value) {
   return (hkey_t)value;
}

Yes, this adds quite a bit of boilerplate in Hashtable.h, but ensures we don't just blindly assume a 1:1 mapping without being explicit about that assumption.

Copy link
Copy Markdown
Contributor

@tanriol tanriol Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to clean it up, shouldn't we go all the way and make the key type a union { pid_t pid; uid_t uid; /* other options */ }?

Copy link
Copy Markdown
Member

@BenBE BenBE Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to not do this as the key type should be numeric. Storing into a union in one field and reading from another is undefined behavior in the C standard and may even leave parts of the memory undefined (when writing uint32_t and reading back uint64_t).

With the suggested approach you could even partially remap entries to improve cache hit patterns by adjusting individual hash functions (e.g. most UIDs are sequential in a certain range), while memory (page) addresses jump in steps of 4KiB). Currently we handle both of these sets of key inputs identically.

Copy link
Copy Markdown
Member

@BenBE BenBE May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice if this would be either left in it's original state or updated to make key space conversions/hashing explicit as elaborated in my comment from last year …

typedef uint32_t ht_key_t;

typedef void(*Hashtable_PairFunction)(ht_key_t key, void* value, void* userdata);

Expand All @@ -35,8 +36,8 @@ void Hashtable_put(Hashtable* this, ht_key_t key, void* value);

void* Hashtable_remove(Hashtable* this, ht_key_t key);

void* Hashtable_get(Hashtable* this, ht_key_t key);
void* Hashtable_get(const Hashtable* this, ht_key_t key);

void Hashtable_foreach(Hashtable* this, Hashtable_PairFunction f, void* userData);
void Hashtable_foreach(const Hashtable* this, Hashtable_PairFunction f, void* userData);

#endif