Hashtable code improvements

[Explorer09]

No description provided.

[BenBE]

The only reason this is safe from overflows is that sizeof(HashtableItem) > 7 …
But I'm not sure, this can also necessarily be said about sizeof(HashtableItem) > 10, thus the multiplications here technically should be overflow-checked …

[Explorer09]

sizeof(HashtableItem) is currently 12 for 32-bit systems and 24 for 64-bit systems. Since htop doesn't support 16-bit systems, I really doubt there would be a case where sizeof(HashtableItem) can be less than 10.

I can add a sizeof(HashtableItem) > 10 assertion just to be safe.

[BenBE]

TIA.

[Explorer09]

I am tempted to adjust this line so that it becomes Hashtable_setSize(this, this->size + 1);

Since Hashtable.size is supposed to be a prime number close to a power of 2. There can be a case where multiplying the size by 2 skips an order of magnitude on the buffer size allocation. Example: (2^14 - 3) * 2 = 2^15 - 6 > 2^15 - 19, thus (2^15 - 19) might be skipped in the buffer size allocation.

I just doubt if this change is safe.

[BenBE]

On first glance this should work. Not tested in detail though. But given that setSize rounds up to the next prime anyway this should likely work.

[Explorer09]

Not sure if I can ask @cgzones a question:

Is it allowed for the assertion of this line be changed to assert(this->size >= this->items);?

In other words, what would happen if this->items == this->size?

There is a commit, b45eaf2, that changed the minimum size to 7, but the reason stated in that commit didn't fully make sense to me. While between 2 and 3, the grow factor ((3 - 2)/2 = 50%) is indeed less than 70%, but between 3 and 7, there'd be no problem with the grow factor ((7 - 3)/3 = 133%). The cause of the assertion error was more of an off-by-one from the conditional 10 * this->items > 7 * this->size. It should be 10 * (this->items + 1) > 7 * this->size instead if we have to satisfy the assertion this->size > this->items.

I am reluctant to add a +1 to the this->items conditional above, as I guess that the whole Hashtable structure should work fine if we allow this->items == this->size.

[BenBE]

Given size is the number of allocted entries and items the number of actually used entries, the >= should be fine.

The reason for avoiding hash tables below 7 is efficiency: It doesn't make sense to allocate smaller blocks as most of our hash tables are far larger anyway.

[Explorer09]

@BenBE I know efficiency is a good reason, but I just want to write a good technical reason in the code comments. Especially that some numbers in the primeDiffs array (in my commit) will be intentionally unused.

[BenBE]

Another (technical) reason is memory fragmentation. Allocating small blocks tends to fragment memory far more than using larger blocks (which for small collections might not even need resizing).

[BenBE]

How's the last commit related?

[Explorer09]

How's the last commit related?

My improvement on the Hashtable code is to hope that the ht_key_t type can be upgraded from unsigned int to size_t, otherwise it would make no sense to support a Hashtable size of more than 2^32 entries.

The last commit might look distracting to the code improvement commits. I apologize. If the last commit would need more review, I'm happy to move it to a separate pull request.

[BenBE]

NP with the commit itself. Just wondered if they are complete. Also, do the current set of changes in the first 3 commits do work without the last one.

[Explorer09]

NP with the commit itself. Just wondered if they are complete. Also, do the current set of changes in the first 3 commits do work without the last one.

I think the last commit needs some cleanup or discussion, but the first 3 commits are ready and can be cherry-picked to main early.

The last commit depends on the first 3 commits but the first 3 can work without the last.

[BenBE]

Can you split off the last commit into its own PR? TIA.

[BenBE]

AFAICS the Hashtable_setSize should be called in either path to allocate new entries as needed.

[Explorer09]

What do you mean? This part of code addresses expanding the buffer. The buckets buffer is allocated stating at Hashtable_new.

[BenBE]

Yes, but once the items get near the size because it can't allocate any more buffer space, you could at some point reach items == size, and thus the next insert will fail due to no more space allocated.

Instead when nearing the maximum capacity we should fall of to a more linear allocation regime …

[Explorer09]

@BenBE When it "can't allocate any more buffer space" htop will exit, because of the xCalloc call.

The case where items == size can only happen on small sizes such as 2 or 3 (the minimum size is 7 now, so the sizes of 2 and 3 are theoretical situations), but even when that happens, the next Hashtable_put call will always grow the buffer. Thus there's no problem here.

[BenBE]

I was more thinking for very large allocations. Will have to take a closer look after New Year's …

[BenBE]

Any harm in making the call to Hashtable_setSize unconditional? The above bounds check should be part of that function already.

[Explorer09]

I was thinking how that would affect the shrinking of the buffer, with respect to this code:

https://github.com/Explorer09/htop-1/blob/3dc65f62da56befae7ed7dcb6e66ca5bea856710/Hashtable.c#L292

I personally like the idea, by centralizing the conditionals that readjust the buffer size, we can save some sanity checks in the Hashtable_setSize function.

[Explorer09]

Update: It seems that there's a side effect if I try to merge the conditionals of expanding and shrinking the buffer in Hashtable_setSize, thus I have to give up on the idea.

When creating a Hashtable through Hashtable_new, it is allowed to specify a larger size for initial allocation. During the initial population of the items, this avoids unnecessary expansion or relocation of the buffer. If I move the shrinking condition to Hashtable_setSize, then the buffer will shrink automatically when adding an element to it. This would remove the benefits of initialing a Hashtable with larger size.

[BenBE]

I think we arrived at this issue before … IIRC.

Maybe inhibit shrinking while we try to insert items …

[Explorer09]

Maybe inhibit shrinking while we try to insert items …

Inhibit shrinking means a flag argument in Hashtable_setSize. It seems like we cannot have less than 2 arguments for the setSize function. If we cannot reduce the number of arguments for it, then I'd like to keep the current function prototype, and use the size argument to determine whether the buffer should grow or shrink.

[BenBE]

@Explorer09 Any reason against pre-computing the table at compile-time?

#include <stdint.h>

#define B(i,a) (((uint64_t)1ull << (i)) - (a)),

#define A1(I, a0) \
    B((I)+0, a0)
#define A2(I, a0,a1) \
    A1((I)+0, a0) \
    A1((I)+1, a1)
#define A4(I, a0,a1,a2,a3) \
    A2((I)+0, a0,a1) \
    A2((I)+2, a2,a3)
#define A8(I, a0,a1,a2,a3,a4,a5,a6,a7) \
    A4((I)+0, a0,a1,a2,a3) \
    A4((I)+4, a4,a5,a6,a7)
#define A16(I, a0,a1,a2,a3,a4,a5,a6,a7,a8,a9,a10,a11,a12,a13,a14,a15) \
    A8((I)+0, a0,a1,a2,a3,a4,a5,a6,a7) \
    A8((I)+8, a8,a9,a10,a11,a12,a13,a14,a15)

static const uint64_t primeDiffs[] = {
    A16( 0, 0,0,1,1,3,1,3,1,5,3,3,9,3,1,3,19)
#if SIZE_MAX > UINT16_MAX
    A16(16, 15,1,5,1,3,9,3,15,3,39,5,39,57,3,35,1)
# if SIZE_MAX > UINT32_MAX
    A16(32, 5,9,41,31,5,25,45,7,87,21,11,57,17,55,21,115)
    A16(48, 59,81,27,129,47,111,33,55,5,13,27,55,93,1,57,25)
# endif
#endif
};

#undef A16
#undef A8
#undef A4
#undef A2
#undef A1
#undef B

Saves you from calculating things at runtime on every access to that table and the memory size isn't of much concern here.

FWIW, the code path isn't hot enough to gamble on keeping a single dcache line for the table contents.

[Explorer09]

Explorer09 Any reason against pre-computing the table at compile-time?

Why pre-compute then? It was because the code path isn't a hot one that I proposed reducing the table size in memory. It's quite trivial to compute ((1ull << n) - A[n]). You don't reallocate the memory of Hashtables often.

The shrunk lookup table size could be just 64 bytes. If I expand and pre-compute the way you do, then it would become 512 bytes (64 * sizeof(uint64_t)) it's 8 times difference.

[BenBE]

Multiple reasons:

• Reducing runtime code complexity (basically a follow-up to PR Allow for optimizing out bound check on 64 bit systems #1909)
• No necessity to safe every last byte; in particular with static (constant) program memory.
• Single-byte access is somewhat less efficient here (additional movzx compared to plain memory read)

[Explorer09]

Multiple reasons:

• Reducing runtime code complexity (basically a follow-up to PR Allow for optimizing out bound check on 64 bit systems #1909)
• No necessity to safe every last byte; in particular with static (constant) program memory.
• Single-byte access is somewhat less efficient here (additional movzx compared to plain memory read)

I don't like the reasoning, although I think such debate is going to be like arguing the color of a bikeshed, and thus I'm not going to change the decisions of the maintainers. You have the choice. I've rebased this PR so you can leave out the lookup table shrink.

Anyway, here's my motivation: My vision of htop is that it should be a compact tool for process monitoring even though it comes with a fancy, terminal UI. That means if there's any chance to reduce memory footprint for the htop program itself, I would wish it could be done (unless there's a larger performance tradeoff with reducing the code size). The memory is better left for large, server applications that need them. In my opinion, it's better for htop to be slightly slower (i.e. take slightly more CPU time) for certain tasks if doing them faster would consume memory that could be essential for server apps.