Switch hashmap implementation to open addressing #301
Conversation
There was a problem hiding this comment.
I noticed the PR description includes some performance numbers, which is great. However, the commit message only vaguely mentions "slightly higher memory usage" and "significantly improved execution time." Could you include the actual measurements in the commit message as well?
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
icgmilk
force-pushed
the
fix_hashmap
branch
from
4d3545d to
c948a44
Compare
icgmilk
force-pushed
the
fix_hashmap
branch
2 times, most recently
from
bf383cf to
f513a1c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
icgmilk
force-pushed
the
fix_hashmap
branch
from
f513a1c to
b1b3230
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
icgmilk
force-pushed
the
fix_hashmap
branch
from
b1b3230 to
f4943ae
Compare
|
Just out of curiosity, how did you test the correctness of this patch? |
I validated this patch with shecc's test suite ( |
Rebase and validate carefully. |
There was a problem hiding this comment.
The last sentence of the commit message specifically mentions that delete is not supported. However, IIUC, we've never had a requirement for this functionality. Therefore, I'm not sure why it's so special that we need to explicitly mention something that has never been used or implemented.
Additionally, I saw in the PR's detailed description that your "saved 30 ms" was measured by running ./out/shecc src/main.c. However, the commit message doesn't mention this; it only states it's "30 ms faster." IMHO, without clarifying how this 30 ms was measured, this number is meaningless.
I mentioned the delete state in my commit message originally because of a comment from cubic-dev-ai, I'll remove that note from the commit message later. I'll also add the measurement details in the commit message. |
Previously, the chaining implementation incurred pointer chasing and per-node allocations, leading to poor cache locality and longer chains under collisions. Open addressing uses a single contiguous array and removes per-node allocations. We cap the load factor at 50% to keep probe lengths short and reduce clustering. This change trades ~320 kB of memory for ~30 ms faster execution (measured with uftrace and usr/bin/time on out/shecc src/main.c), primarily due to better cache locality.
|
Thank @icgmilk for contributing! |
4 participants
Previously, the chaining implementation incurred pointer chasing and per-node allocations, leading to poor cache locality and longer chains under collisions.
Open addressing uses a single contiguous array and removes per-node allocations. We cap the load factor at 50% to keep probe lengths short and reduce clustering.
This change trades ~320 kB of memory for ~30 ms faster execution, primarily due to better cache locality.
Validated with shecc’s test suite (
make check).Notice that current implementation lacks delete state as there is no in-place delete use case.
Performance analysis for out/shecc src/main.c
Using
/usr/bin/time -vand uftrace to benchmark memory usage and execution time.Chaining
/usr/bin/time -vuftrace
Open Addressing
/usr/bin/time -vuftrace
Summary by cubic
Switch hashmap from separate chaining to open addressing with linear probing, rehashing at 50% load. This improves cache locality and delivers a small, consistent speedup with similar memory usage.
Refactors
Performance