Skip to content
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.

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

Double the width of static allocations for empty tables #596

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

See this discussion here: #578 (comment)

Effectively, the current code will actually need Group::WIDTH + 1 bytes for an empty table, but we only allocate Group::WIDTH bytes for it. This could potentially invoke UB based upon the invariants we set in the table, but because we happen to short-circuit empty tables in just the right ways in the actual code, we never actually trigger it.

Since the static allocations are aligned to the group width, we double the size returned, since it would otherwise just add that much extra padding anyway. And since the size of the static allocations added by this is effectively insignificant (8-16 bytes depending on the implementation chosen), it's fair to just do this anyway to make the code more robust.

@Amanieu
Copy link
Member

Amanieu commented Dec 10, 2024

I still don't think this is needed: the only place that needs WIDTH instead of WIDTH - 1 trailing control bytes is set_ctrl, and that requires write access which doesn't work with the static empty table anyways. The only other places which access the trailing control bytes are where Group::load is called and that only accesses WIDTH - 1 trailing bytes since the input index is always less than num_buckets.

@clarfonthey
Copy link
Contributor Author

I think perhaps the biggest issue here is that bucket_mask can't differentiate between zero and one element, and so maybe I'm trying to jump the gun a little too soon to make bucket_mask dictate the size properly when we really should be storing len instead. But also, I'm not sure that's right either.

I think that we should be consistent in making sure what guarantees the table actually stores, but you're right that this is probably the wrong approach. I'll close this for now.

@Amanieu
Copy link
Member

Amanieu commented Feb 2, 2025

I do want to keep bucket_mask as it is because it is part of the critical path for hash table lookups. Every CPU cycle of latency between calculating a hash value and loading the control bytes matters.

@clarfonthey
Copy link
Contributor Author

I mean, that's fair, and it's been long enough that I've looked into the code that I really should go back to figuring out why exactly I wanted this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants