Skip to content
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

feat: add a method for resetting index state #31

Closed
wants to merge 1 commit into from
Closed

Conversation

EvanHahn
Copy link
Contributor

@EvanHahn EvanHahn commented Jan 18, 2024

I recommend reviewing this with whitespace changes disabled.

This adds MultiCoreIndexer.prototype.removeCoreAndUnlinkIndexStorage(core) which removes the core from the indexer and destroys the storage.

This felt a bit complicated, so I'm keen to hear feedback on that in particular (but also anything else!).

Addresses #26.

This adds
`MultiCoreIndexer.prototype.removeCoreAndUnlinkIndexStorage(core)` which
removes the core from the indexer and destroys the storage.

Addresses [issue #26][0].

[0]: #26
Comment on lines +62 to +65
for (const stream of this.#streams.keys()) {
if (!stream.drained) return false
}
return true
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr: this is slower but I think this is fine, as long as we don't have tons of cores.

Something needed to change here. If you remove a stream, we might need to change the state of drained; a simple boolean is insufficient for this purpose.

A few options I considered:

  1. Make the value computed and remove state. Pros: reduces the amount of state that can go wrong. Con: a performance cost; the operation goes from O(1) to O(N).
  2. Make the state more complex: probably a Set<CoreIndexStream> of un-drained streams. Pros: roughly as fast as the previous approach. Con: requires more delicate state management.
  3. Make the state computed, but cache the value and mark it "dirty". This would probably mean that #drained was of type undefined | boolean. Pros: a hybrid of the previous approaches. Cons: still incurs a performance hit the first time you read.

I went with the first option because I felt it was the simplest, but option 2 also seems reasonable.

If there are a small number of cores, this is probably not an issue. That's confirmed by the benchmarks/multi-core-indexer.js benchmark:

BeforeAfter
# Index 20 cores of 1000 blocks (10 times)
ok ~4.57 s (4 s + 566600718 ns)
# Index 20 cores of 1000 blocks (10 times)
ok ~4.6 s (4 s + 599290806 ns)

However, if there are lots of cores, this might become a problem.

Is what I have here okay?

@EvanHahn EvanHahn marked this pull request as ready for review January 18, 2024 04:03
@EvanHahn EvanHahn linked an issue Jan 18, 2024 that may be closed by this pull request
@EvanHahn EvanHahn marked this pull request as draft January 18, 2024 16:19
@EvanHahn
Copy link
Contributor Author

Decided to take a different approach with #34.

@EvanHahn EvanHahn closed this Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method for resetting index state
1 participant