Hashtable: only shrink in Put and fix various other issues

[charlievieth]

Changes:

• Only resize the hash in Hashtable_put to reduce thrashing, otherwise
  we a large deletion results in multiple incremental resizes.
• Reduce the load factor to 2/3 from 0.7 since testing shows the
  average and median probe lengths increases quickly after 2/3 of
  capacity is used.
• Eliminate modulo math when incrementing the index in put, get, and
  remove.
• Increase the number of available prime sizes.
• Define the hash key type as uint32_t instead of "unsigned int", which
  is stable across platforms and large enough to hold any PID.
• Change Hashtable_get to take a "const" Hashtable.

This was originally meant to fix an issue with the Hashtable shrink
factor that was fixed with 230dc9c.

[BenBE]

Actually trying to run the code produces an infinite loop in Hashtable_put because the resizing doesn't trigger. Can you have another look?

[BenBE]

LGTM. Mostly cosmetic changes left.

[BenBE]

Prefer normal multiplication.

Suggested change

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

[BenBE]

Another point that should be integrated into this patch series …

[BenBE]

Unnecessary parens

Suggested change

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

[BenBE]

Further code style stuff that is not resolved …

[BenBE]

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.

[tanriol]

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?

[BenBE]

This is still open and should be resolved.

[charlievieth]

@BenBE Let's hold off on these changes for a moment. I was just reviewing the implications of only shrinking on Hashtable_put() and one of the side-effects is that a Hashtable created with an explicit size greater than MIN_TABLE_SIZE will shrink on the first call to Hashtable_put, which we obviously don't want.

The downside of moving the shrink logic back to Hashtable_remove is that when turnover is high we'd end up shrinking the table on removes only to grow it again on puts (basically doublind the work we have to do).

Have we considered changing the hash table to not automatically shrink? This would allow for adding something like Hashtable_reserve(N) (to reserve space for N items before inserts) and we could use the same compact logic that we use for Vectors (which we never shrink)?

[BenBE]

Couldn't you achieve this effect by setting the shrink threshold very low? The performance of the hash table won't be hurt if it is "too empty", thus down-sizing it late should have no negative impact.

As per an actual Hashtable_reserve call: I'd appreciate such a function but this is likely best left as a separate PR as the involved code changes will be quite substantial.

[tanriol]

The downside of moving the shrink logic back to Hashtable_remove is that when turnover is high we'd end up shrinking the table on removes only to grow it again on puts (basically doublind the work we have to do).

What are the cases for high turnover in htop - fighting off a fork bomb manually?

Furthermore, this likely won't be a problem for the current use in ProcessList because ProcessList_scan first adds all new processes and after that removes the expired ones, so in stable system state it would go n -> 2n -> n in worst case. Sure, we can still have resizes with highly variable process count, but that would trigger them in the Hashtable_put variant too.

[tanriol]

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?

[tanriol]

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).

[BenBE]

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.

[tanriol]

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.

[BenBE]

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.

[tanriol]

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 */ }?

[BenBE]

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.

[BenBE]

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 …

[BenBE]

There are still several things unresolved that have been noted in previous reviews. Please fix them.

If you could update the existing commits by squashing them/rebasing fixes, this would be much appreciated.

[BenBE]

This is still open and should be resolved.

[BenBE]

Another point that should be integrated into this patch series …

[BenBE]

Further code style stuff that is not resolved …

[BenBE]

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 …