Use 0 for empty value #34
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
Draft
|
This includes changes from #33 |
Avo documentation requires version to be pinned in order to be used in production, however there was no reference to avo in the modules here, so it's not possible to reproduce the build. This extracts avo source to a submodule, in order to prevent it from being downloaded as a transitive dependency when dolthub/swiss is required, and adds a go:generate directive to simplify the build process. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
This doesn't change the logic, but by defining h2 as uint8, and using uint8 elsewhere, we can work properly with higher bits (and define empty in it's binary form, instead of having to use the negative value and a comment). This will also allow us future changes without having to play with overflows. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
colega
force-pushed
the
empty-as-0
branch
3 times, most recently
from
904b847
to
72dab86
Compare
|
I ran the benchmarks in a Github codespace, the results make sense, although the deviation is quite high (expected from a virtual machine somewhere on the cloud). It would still be nice if someone could run these on their machine. |
Current value has to be written for all metadata values when we're instantiating a new map or growing. This changes empty to be the zero byte, and tombstone to be byte 1, by adding 2 (h2Offset) to all hashes. Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
|
This is ready for review. |
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.
In the current implementation we have to initialize all metadata values to the specific
emptyvalue when we're creating a new map or growing it.This changes empty to be the zero byte, and tombstone to be byte 1, by adding 2 to all hashes.
We do have to run one extra sum
+2to ensure thath2hashes don't have the lower bit set, but we compensate that by doing one less operation when matchingemptyin the group, (two less assembly instructions in the amd64 simd code!), so there's no penalty there.I benchmarked on ARM (Apple Silicon), and for some reason some other benchmark results have also improved (we're doing some less work, but not that much less. Maybe some operations are optimized? but why int benchmarks are unaffected then?), I'll run more benchmarks to confirm, but it would be great if someone on amd64 could double check.