Refactor (Hyper)ClockCache code for upcoming changes #11572
Closed
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
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Separate out some functionality that will be common to both static and dynamic HCC into BaseClockTable. Table::InsertState and GrowIfNeeded will be used by the dynamic HCC so don't make much sense right now. One small functional change in insertion has the process abort as soon as it finds a pre-existing match (already_matches & FindSlot). This wasn't strictly needed for performance but cleans up the calling of TryInsert as we now don't try to call it after we've already seen a match. Test Plan: existing tests. TODO: performance tests
pdillinger
force-pushed
the
clock_cache_refactor2
branch
from
8285618
to
65dcd46
Compare
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 11, 2023
Summary: Stacked on facebook#11572 * Minimize use of std::function and lambdas to minimize chances of compiler heap-allocating closures (unnecessary stress on allocator). It appears that converting FindSlot to a template enables inlining the lambda parameters, avoiding heap allocations. * Clean up some logic with FindSlot * Fix handling of rare case of probing all slots, with new unit test. (Previously Insert would not roll back displacements in that case, which would kill performance if it were to happen.) * Add an -early_exit option to cache_bench for gathering memory stats before deallocation. Test Plan: unit test added for probing all slots ## Seeing heap allocations Run `MALLOC_CONF="stats_print:true" ./cache_bench -cache_type=hyper_clock_cache` before facebook#11572 vs. after this change. Before, we see this in the interesting bin statistics: ``` size nrequests ---- --------- 32 578460 64 24340 8192 578460 ``` And after: ``` size nrequests ---- --------- 32 (insignificant) 64 24370 8192 579130 ``` ## Performance test Build with `make USE_CLANG=1 PORTABLE=0 DEBUG_LEVEL=0 -j32 cache_bench` Run `./cache_bench -cache_type=hyper_clock_cache -ops_per_thread=5000000` in before and after configurations, simultaneously: Before: Complete in 33.244 s; Rough parallel ops/sec = 2406442 After: Complete in 32.773 s; Rough parallel ops/sec = 2441019
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 12, 2023
Summary: Stacked on facebook#11572 * Minimize use of std::function and lambdas to minimize chances of compiler heap-allocating closures (unnecessary stress on allocator). It appears that converting FindSlot to a template enables inlining the lambda parameters, avoiding heap allocations. * Clean up some logic with FindSlot * Fix handling of rare case of probing all slots, with new unit test. (Previously Insert would not roll back displacements in that case, which would kill performance if it were to happen.) * Add an -early_exit option to cache_bench for gathering memory stats before deallocation. Test Plan: unit test added for probing all slots ## Seeing heap allocations Run `MALLOC_CONF="stats_print:true" ./cache_bench -cache_type=hyper_clock_cache` before facebook#11572 vs. after this change. Before, we see this in the interesting bin statistics: ``` size nrequests ---- --------- 32 578460 64 24340 8192 578460 ``` And after: ``` size nrequests ---- --------- 32 (insignificant) 64 24370 8192 579130 ``` ## Performance test Build with `make USE_CLANG=1 PORTABLE=0 DEBUG_LEVEL=0 -j32 cache_bench` Run `./cache_bench -cache_type=hyper_clock_cache -ops_per_thread=5000000` in before and after configurations, simultaneously: Before: Complete in 33.244 s; Rough parallel ops/sec = 2406442 After: Complete in 32.773 s; Rough parallel ops/sec = 2441019
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 12, 2023
Summary: ... ahead of planned dynamic HCC variant. This changes simplifies some logic while still enabling future code sharing between implementation variants. Stacked on facebook#11572 Test Plan: existing tests
jowlyzhang
reviewed
Jul 12, 2023
| // required, and the operation should fail if not possible. | ||
| // NOTE: Otherwise, occupancy_ is not managed in this function | ||
| template <class Table> | ||
| Status ChargeUsageMaybeEvictStrict(size_t total_charge, size_t capacity, |
There was a problem hiding this comment.
nit: this and below ChargeUsageMaybeEvictNonStrict were private before, should they continue to have more restricted access.
There was a problem hiding this comment.
Thanks. I'll fix this in #11609
jowlyzhang
approved these changes
Jul 12, 2023
|
@pdillinger merged this pull request in c3c84b3. |
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 12, 2023
Summary: ... ahead of planned dynamic HCC variant. This changes simplifies some logic while still enabling future code sharing between implementation variants. Stacked on facebook#11572 Test Plan: existing tests
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 12, 2023
Summary: Stacked on facebook#11572 * Minimize use of std::function and lambdas to minimize chances of compiler heap-allocating closures (unnecessary stress on allocator). It appears that converting FindSlot to a template enables inlining the lambda parameters, avoiding heap allocations. * Clean up some logic with FindSlot * Fix handling of rare case of probing all slots, with new unit test. (Previously Insert would not roll back displacements in that case, which would kill performance if it were to happen.) * Add an -early_exit option to cache_bench for gathering memory stats before deallocation. Test Plan: unit test added for probing all slots ## Seeing heap allocations Run `MALLOC_CONF="stats_print:true" ./cache_bench -cache_type=hyper_clock_cache` before facebook#11572 vs. after this change. Before, we see this in the interesting bin statistics: ``` size nrequests ---- --------- 32 578460 64 24340 8192 578460 ``` And after: ``` size nrequests ---- --------- 32 (insignificant) 64 24370 8192 579130 ``` ## Performance test Build with `make USE_CLANG=1 PORTABLE=0 DEBUG_LEVEL=0 -j32 cache_bench` Run `./cache_bench -cache_type=hyper_clock_cache -ops_per_thread=5000000` in before and after configurations, simultaneously: Before: Complete in 33.244 s; Rough parallel ops/sec = 2406442 After: Complete in 32.773 s; Rough parallel ops/sec = 2441019
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 14, 2023
Summary: Stacked on #11572 * Minimize use of std::function and lambdas to minimize chances of compiler heap-allocating closures (unnecessary stress on allocator). It appears that converting FindSlot to a template enables inlining the lambda parameters, avoiding heap allocations. * Clean up some logic with FindSlot (FIXMEs from #11572) * Fix handling of rare case of probing all slots, with new unit test. (Previously Insert would not roll back displacements in that case, which would kill performance if it were to happen.) * Add an -early_exit option to cache_bench for gathering memory stats before deallocation. Pull Request resolved: #11601 Test Plan: unit test added for probing all slots ## Seeing heap allocations Run `MALLOC_CONF="stats_print:true" ./cache_bench -cache_type=hyper_clock_cache` before #11572 vs. after this change. Before, we see this in the interesting bin statistics: ``` size nrequests ---- --------- 32 578460 64 24340 8192 578460 ``` And after: ``` size nrequests ---- --------- 32 (insignificant) 64 24370 8192 579130 ``` ## Performance test Build with `make USE_CLANG=1 PORTABLE=0 DEBUG_LEVEL=0 -j32 cache_bench` Run `./cache_bench -cache_type=hyper_clock_cache -ops_per_thread=5000000` in before and after configurations, simultaneously: ``` Before: Complete in 33.244 s; Rough parallel ops/sec = 2406442 After: Complete in 32.773 s; Rough parallel ops/sec = 2441019 ``` Reviewed By: jowlyzhang Differential Revision: D47375092 Pulled By: pdillinger fbshipit-source-id: 46f0f57257ddb374290a0a38c651764ea60ba410
jowlyzhang
pushed a commit
to jowlyzhang/rocksdb
that referenced
this pull request
Jul 18, 2023
Summary: Separate out some functionality that will be common to both static and dynamic HCC into BaseClockTable. Table::InsertState and GrowIfNeeded will be used by the dynamic HCC so don't make much sense right now. Pull Request resolved: facebook#11572 Test Plan: existing tests. No functional changes intended. Performance test in subsequent PR facebook#11601 Reviewed By: jowlyzhang Differential Revision: D47110496 Pulled By: pdillinger fbshipit-source-id: 379bd433322a42ea28c0043b41ec24956d21e7aa
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Jul 18, 2023
Summary: ... ahead of planned dynamic HCC variant. This changes simplifies some logic while still enabling future code sharing between implementation variants. Stacked on facebook#11572 Test Plan: existing tests
facebook-github-bot
pushed a commit
that referenced
this pull request
Jul 18, 2023
Summary: ... ahead of planned dynamic HCC variant. This changes simplifies some logic while still enabling future code sharing between implementation variants. Detail: For complicated reasons, using a std::function parameter to `ConstApplyToEntriesRange` with a lambda argument does not play nice with templated HandleImpl. An explicit conversion to std::function would be needed for it to compile. Templating the function type is the easy work-around. Also made some functions from #11572 private as recommended Pull Request resolved: #11609 Test Plan: existing tests Reviewed By: jowlyzhang Differential Revision: D47407415 Pulled By: pdillinger fbshipit-source-id: 0f65954db16335999b78fb7d2563ec627624cef0
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.
Summary: Separate out some functionality that will be common to both static and dynamic HCC into BaseClockTable. Table::InsertState and GrowIfNeeded will be used by the dynamic HCC so don't make much sense right now.
Test Plan: existing tests. No functional changes intended.
Performance test in subsequent PR #11601