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
Adds poolshare index for BucketsDB #4224
Conversation
src/bucket/BucketIndex.h
Outdated
@@ -37,6 +37,9 @@ class BucketManager; | |||
// BucketIndex abstract interface | |||
class BucketIndex : public NonMovableOrCopyable | |||
{ | |||
protected: | |||
static const inline std::vector<PoolID> kEmptyVec = {}; |
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.
This doesn't follow the naming style. Also I don't think it needs to be a class member as it's only used in a single function (so could be defined right there).
@@ -45,7 +49,7 @@ template <class IndexT> class BucketIndexImpl : public BucketIndex | |||
void | |||
load(Archive& ar) | |||
{ | |||
ar(keysToOffset, filter); | |||
ar(assetToPoolID, keysToOffset, filter); |
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.
Is there a risk of loading the old version with the new code (or vice versa)?
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.
No, in BucketIndex::load
, we load the version and pageSize first. These two values are always serialized first no matter the index version. Then we check the index version, and if the on-disk version number doesn't match the expected version number, we drop the on-disk index and reindex from scratch.
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.
should we delete the outdated index right away instead of keeping both until the bucket gets cleaned up? (so that we don't accidentally use it)
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.
We don't actually have both indexes persisted, but we generate a new index, then rename the new index the the same name as the old index. On linux, this deletes the old file. However, I looked up in the standard and this behavior is implementation defined, so I agree we should delete explicitly here.
d79faca
to
6656553
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!
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!
@@ -45,7 +49,7 @@ template <class IndexT> class BucketIndexImpl : public BucketIndex | |||
void | |||
load(Archive& ar) | |||
{ | |||
ar(keysToOffset, filter); | |||
ar(assetToPoolID, keysToOffset, filter); |
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.
should we delete the outdated index right away instead of keeping both until the bucket gets cleaned up? (so that we don't accidentally use it)
6656553
to
66c6f1f
Compare
r+ 66c6f1f |
Description
Resolves #4223
This extends
BucketIndex
to include a map ofAsset -> poolID
in order to optimizeloadPoolShareTrustLinesByAccountAndAsset
. This change improves query performance by around ~2500x and minimally impacts memory overhead. I suspect the memory increase is approximately 1.5 MB, but this is so small that I was not able to measure a change during testing.Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)