optimize null lookup for StaticStringMap #22967
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently the initLenIndexes function populates the len_indexes
array with values (which are indexes into the kvs array) of the
form:
{0, 0, 0, 0, .. 1, 1, 1, .. 2, 2, 2 ....}
where the repeated indexes are the "missing sizes", for example
if the keys are {"ab", "abcdefghi"} the len_indes array looks
like:
{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}
When you lookup say "none", will compare first with kvs.keys[0]
which is "ab" then in the second loop iteration with kvs.keys[1]
which does return null.
The proposed change replaces these repeated indexes for the
u32 max-value sentinel which indicates that for that length
there are no entries in the kv array. In the case above then
it does an early without having to fetch kvs.keys[i].
The speedup is expected to be minimal, unless there are lots
of null lookups or the kvs array is in a contested cache line.
An alternative is to use 0 as sentinel, which means the empty
string could not be used as a key, which is a breaking ABI
change.
A test is added to make explicit that the "" key behavior
is not accidentally broken.
andrewrk
requested changes
Feb 22, 2025
Currently the initLenIndexes function populates the len_indexes
array with values (which are indexes into the kvs array) of the
form:
{0, 0, 0, 0, .. 1, 1, 1, .. 2, 2, 2 ....}
where the repeated indexes are the "missing sizes", for example
if the keys are {"ab", "abcdefghi"} the len_indes array looks
like:
{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}
When you lookup say "none", will compare first with kvs.keys[0]
which is "ab" then in the second loop iteration with kvs.keys[1]
which does return null.
The proposed change replaces these repeated indexes for the
u32 max-value sentinel which indicates that for that length
there are no entries in the kv array. In the case above then
it does an early without having to fetch kvs.keys[i].
The speedup is expected to be minimal, unless there are lots
of null lookups or the kvs array is in a contested cache line.
An alternative is to use 0 as sentinel, which means the empty
string could not be used as a key, which is a breaking ABI
change.
A test is added to make explicit that the "" key behavior
is not accidentally broken.
|
I have updated the code to use a constant. I could not figure out how to use an enum (and not look ugly) for this case. As for the performance, that sent me into a long adventure. I've written a benchmark and the results are here The TLDR is that is tiny improvement, hard to measure specially in my M1, but its there. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the initLenIndexes function populates the len_indexes array with values (which are indexes into the kvs array) of the form:
{0, 0, 0, 0, .. 1, 1, 1, .. 2, 2, 2 ....}
where the repeated indexes are the "missing sizes", for example if the keys are {"ab", "abcdefghi"} the len_indexes array looks like:
{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}
When you lookup say "none", it will compare first with kvs.keys[0] which is "ab" then in the second loop iteration with kvs.keys[1] which does return null.
The proposed change replaces these repeated indexes for the u32 max-value sentinel which indicates that for that length there are no entries in the kv array. In the case above then it does an early without having to fetch kvs.keys[i].
The speedup is expected to be minimal, unless there are lots of null lookups or the kvs array is in a contested cache line.
An alternative is to use 0 as sentinel, which means the empty string could not be used as a key, which is a breaking ABI change.
A test is added to make explicit that the "" key behavior is not accidentally broken.