-
Notifications
You must be signed in to change notification settings - Fork 583
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(memory): use thread-local sequence-based memory eviction policy #16087
Conversation
Signed-off-by: MrCroxx <[email protected]>
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.
license-eye has totally checked 4973 files.
Valid | Invalid | Ignored | Fixed |
---|---|---|---|
2138 | 1 | 2834 | 0 |
Click to see the invalid file list
- src/common/benches/bench_sequencer.rs
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Hi, would you mind sharing more information on the motivation and methodology in the PR description? |
Signed-off-by: MrCroxx <[email protected]>
4a8785b
to
7ebc5d3
Compare
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
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
} | ||
|
||
pub fn put(&mut self, key: K, mut value: V) -> Option<V> { | ||
unsafe { |
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.
So many unsafe
in this file 🥵 Please explain the necessities of unsafe
with some comments in this file e.g. why LinkedList
can't satisfy this use case.
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.
Also, I tend to wrap every linked list operations in unsafe { ... }
instead of simply wrapping all the function body code. It makes it hard to reason about the safety.
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.
The single thread LRU with sequence implementation is basically ported from our original modified lru repo, foyer, and our original block cache implementation (ported from RocksDB).
A LRU cache is a classic multi-indexer problem, which cannot be achieved easily and cheaply with safe Rust. It requires mutability with shared pointers, O(1) node lookup with address or reference (which cannot be achieved with std linked list) .
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.
It requires mutability with shared pointers
Yeah, got this. But is it possible to reduce the size of unsafe
block? As mentioned in the 2nd comment.
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.
Overall the idea LGTM
src/common/Cargo.toml
Outdated
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.
I heard that some stateless queries in NexMark were negatively affected by this PR for some "unknown" cause. Have we found the reason now?
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.
Still not. But the regression hasn't appear these weeks.
Is it necessary to also run the 4X (32C 64G) nexmark once? https://buildkite.com/risingwave-test/nexmark-benchmark/builds/3664#018f689a-0b1d-4fc3-9b82-912358895ccc |
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Wha's the hardware configuration of the longevity test? I've ran longevity test and there is no regression. |
Signed-off-by: MrCroxx <[email protected]>
Each MV in longevity uses 3 as the parallelism. |
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.
Overall LGTM
} | ||
|
||
pub fn put(&mut self, key: K, mut value: V) -> Option<V> { | ||
unsafe { |
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.
It requires mutability with shared pointers
Yeah, got this. But is it possible to reduce the size of unsafe
block? As mentioned in the 2nd comment.
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Signed-off-by: MrCroxx <[email protected]>
Discussed offline. Separating the unsafe blocks barely helps reduce the explosion radius. Let's keep it as it is now. |
…16087) Signed-off-by: MrCroxx <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Motivation
Resolves #15305
The previous memory eviction strategy was based on epoch, and in the following scenarios, there may be cases of excessively aggressive eviction:
This PR introduces a sequence-based memory eviction mechanism. The eviction of the cache is no longer based on epochs but on the sequence of cache access allocation with finer granularity.
Because the sequence needs to be globally shared, although only one atomic variable is needed for the sequence, the overhead of cache invalidation caused by frequent
fetch_add
cannot be ignored. Therefore, this PR introduces athread_local
sequence that allows global reordering within a certain range.When insert/access an entry into/from the managed lru cache, the managed lru cache acquires a sequence from the
Sequencer
. TheSequencer
use a thread-local variable to grant the sequence. the thread-local variable is synchronized with the global sequence if (a) the pre-allocated local sequences (step
) are exhausted, or (b) the local sequence lag is higher than the threshold (lag
). When evicting, the memory controller calculate the memory ratio to evict and normalize it as watermark sequence with the global sequence. The out-of-order threshold ismax(lag, step)
.CN node memory (before vs after):
Changes
lru
dependency. Use customized implemention inrisingwave_common::lru
.risingwave_common::sequencer
.factor
configuration for each eviction policy to control the eviction speed.Configurations
Micro Benchmarks for Components
Sequencer
microbench:lru microbench:
Benchmark
benchmark (nexmark, vs nightly-20240511):
http://metabase.risingwave-cloud.xyz/question/2219-nexmark-rw-compare?risingwave_tag_1=nightly-20240512&rw_label_1=daily&risingwave_metrics=avg-source-output-rows-per-second&risingwave_tag_2=git-8bc7ee189094e72c65db0725f05263ca3ec08be3&rw_label_2=benchmark-xx-tls
benchmark (nexmark, vs main without this PR):
http://metabase.risingwave-cloud.xyz/question/2219-nexmark-rw-compare?risingwave_tag_1=git-91b7ee29ce4d846f9c2ee6d9f56264bab414250a&rw_label_1=benchmark-xx-main&risingwave_metrics=avg-source-output-rows-per-second&risingwave_tag_2=git-8bc7ee189094e72c65db0725f05263ca3ec08be3&rw_label_2=benchmark-xx-tls
Checklist
./risedev check
(or alias,./risedev c
)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.