ignore --shard-stores if --compact is present #660
Merged
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
A future commit will make it safe to do so, but it's still memory-inefficient to use --compact on a non-renumbered PBF
CompactNodeStore doesn't know how to compute if it contains a node, which is a prerequisite for sharding. The two settings don't make much sense together: sharding will create N CompactNodeStores, which each will take as much memory as a single one, since each will likely have a large node ID. This differs from BinarySearchNodeStore and SortedNodeStore, where each of the N store instances will take roughly 1/N memory. Instead: - fail faster and more clearly by throwing if CompactNodeStore.contains is called - don't enable sharding if --compact is passed
|
Thanks! Yes, that makes sense. Using |
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.
Fixes #657
These two settings are incompatible, because CompactNodeStore does not implement
.contains(...)Instead, if
--compactis present, don't shard stores. Further, throw if code calls.contains(...), so that we'll fail noisily and more clearly.I debated teaching CompactNodeStore how to do
.contains(...), e.g. by maintaning a bitmap of which entries were present. Ultimately, though, I think sharding CompactNodeStores does not make sense -- each CompactNodeStore instance will use about the same amount of memory (vs the other stores, which will each use 1/N memory)Also a small tweak: detect if
--compactis used on a non-renumbered PBF, and warn the user.