-
Notifications
You must be signed in to change notification settings - Fork 51
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
perf: Add badger multi scan support #85
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I had some questions/issues a few days ago when I first reviewed, but either I missed some files/changes or I was just tired as It all make sense now!
Will need to rebase and resolve any (potential) linter issues
Nice cheers John :) |
Just to confirm, this is blocked by benchmark stuff correct @AndrewSisley ? |
Correct - it changes some badgerDb params which might impact performance |
74f67a3
to
2038ce4
Compare
Codecov Report
@@ Coverage Diff @@
## develop #85 +/- ##
===========================================
- Coverage 59.06% 58.71% -0.35%
===========================================
Files 87 89 +2
Lines 8344 8531 +187
===========================================
+ Hits 4928 5009 +81
- Misses 2890 2980 +90
- Partials 526 542 +16
|
4979346
to
3fbe92c
Compare
Locally run benchmarks for dev branch:
|
Locally run benchmarks for this branch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor issue with the iterable
naming / package organization (details in the comments).
I think the mutex is overkill to protect against the race condition, but we can track that in a diff issue.
54269ea
to
a79d683
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Awesome stuff, and the potential speedup is really encouraging!
len=1 handled in MergeAscending, no need for the extra code-branch
Lines are unchanged, not sure why flagged now
a79d683
to
b93bc96
Compare
* Close query when updating * Close fetcher on collection get * Simplify spans logic len=1 handled in MergeAscending, no need for the extra code-branch * Add proper multi-scan support for badger transactions * Fix linter errors Lines are unchanged, not sure why flagged now
* Close query when updating * Close fetcher on collection get * Simplify spans logic len=1 handled in MergeAscending, no need for the extra code-branch * Add proper multi-scan support for badger transactions * Fix linter errors Lines are unchanged, not sure why flagged now
Closes #60
Adds support for iterating through multiple badger prefixes and defines the interfaces for use by other datastores.
Tested the new shim by flipping the tests back to the Map datastore - found two production bugs in doing so (fixed with commits
Move discard to after error check
andFix if-else
).Suggest not merging until bench marked - is a decent sized change, and it also disables badger's PrefetchValues - the performance impact of which I do not know.