Fix micro benchmarks #538
Merged
Fix micro benchmarks #538
Conversation
This file contains hidden or 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
|
Related: #440 |
The minimum length of keys for the compact index increased from 6 to 8 bytes at some point. Also, releasing a run was changed to remove the files associated with it. The cleanup code was manually doing the same, which then became unnecessary and started causing issues. Merge micro bench: release run during cleanup only
This previously tried creating the wbblobs file with an empty path, so there was an existing directory (the benchmark's root directory) already where the file was supposed to go.
With recent changes related to WriteBufferBlobs, flushing a write buffer started copying its blobs to a new file. This made the setup fail, since it generated random blob references, but no actual blobs they point at.
This fixes the file paths errors for the write buffer blobs. It is also generally the right direction to go: If we ever want these tests to work with a write buffer and blobs, lookups must look at the original write buffer blobs, not an empty one.
mheinzel
force-pushed
the
mheinzel/fix-benchmarks
branch
from
eb7d34d to
a7114d1
Compare
mheinzel
requested review from
dcoutts,
jorisdral,
recursion-ninja and
wenkokke
as code owners
jorisdral
reviewed
Jan 23, 2025
| -- We make sure to immediately close resulting runs so we don't run | ||
| -- out of file handles or disk space. However, we don't want it to | ||
| -- be part of the measurement, as it includes deleting files. | ||
| -- Therefore, ... TODO |
There was a problem hiding this comment.
There is a dangling TODO here
There was a problem hiding this comment.
Oops, that slipped through. Thanks for pointing it out. #547
| randomEntry g = frequency [ | ||
| (20, \g' -> let (!v, !g'') = uniform g' in (Insert v, g'')) | ||
| , (1, \g' -> let (!v, !g'') = uniform g' | ||
| (!b, !g''') = genBlobSpan g'' | ||
| (!b, !g''') = randomByteStringR (0, 2000) g'' -- < 2kB |
There was a problem hiding this comment.
You can generate much smaller blobs. It would speed up the benchmark setup, and the performance of the lookups code does not depend on the blob size. But it's maybe also not super important because inserts with blobs are generated only rarely
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jan 24, 2025
Quick follow-up to address feedback for #538
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.
Some micro benchmarks don't run any more. I fixed two where the benchmark environment code got broken by recent changes.
Fixes #440. (this was one of them)