A table handle rebuilt its own map key on every request - #363
Open
bjmeetsfo wants to merge 4 commits into
Open
Conversation
Every execute and batch_execute asks the client for the table's LIVE
options -- the shard count it has now, not the one the handle was opened
with. That read went through a Mutex, so requests on different threads
queued behind each other for it.
Adding threads made the client slower. Measured with drop_percent 100, so
execute returns client-side and what is left is the client's own work:
threads before after
1 2.72 M/s 3.99 M/s
2 3.47 M/s 5.52 M/s
4 1.55 M/s 6.36 M/s
8 1.16 M/s 5.60 M/s
Before, throughput peaked at two threads and fell to about a third of that
by eight -- adding cores took work away. After, it rises through four and
holds. Three before-runs and two after-runs, interleaved on a shared box:
the absolute numbers move with the load, the shape does not.
It is a read, so it is an RwLock now and the readers run together. The
writers -- topology sync, open and close -- are rare and still exclusive.
The map key was also rebuilt, and allocated, on every one of those reads,
from two fields that never change for the life of a handle. The handle
carries it now.
`cached_table` was reading the map too, so it shares the lock rather than
excluding everyone from it.
# Conflicts: # crates/temporalstore-rust/src/client.rs # crates/temporalstore-rust/src/client/client_meta_sync.rs
While this was open, main converted the client's table map from a mutex to a reader-writer lock, and chose a read lock at the sites that only read. That is this branch's first half, done better -- this branch used a write lock at those sites, which is correct but heavier. Main's side is kept everywhere they disagreed. What main does not have is the second half. A table handle's key into the client's table map is its namespace and table name joined, neither of which changes for the life of the handle, and it was being rebuilt and allocated on every request that asked for the live options, plus once more on every write that checked whether the topology was due a refresh. The handle now carries it. No site rebuilds it. This is why the branch is smaller than it was: one file instead of two, because half of it is already on main.
bjmeetsfo
changed the title
…ons-are-a-shared-read
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This branch is now half the size it was, because main landed the other half.
While it was open, main converted the client's table map from a mutex to a reader-writer lock, and chose a read lock at the sites that only read. That was this branch's first half, done better: this branch used a write lock at those sites, which is correct but heavier. Main's side is kept everywhere the two disagreed.
What is left, and why it is still worth landing
A table handle's key into the client's table map is its namespace and table name joined. Neither changes for the life of the handle, and it was being rebuilt -- and allocated -- on every request that asked for the live options, and once more on every write that checked whether the topology was due a refresh.
The handle now carries it, built once when the handle is made. No site rebuilds it.
Verification
cargo check --all-targetsclean; client 35, metadata 326, proxy 75, end-to-end 10, all passing.The surviving change is one file: the field, the two places a handle is built, and the three lookups that used to rebuild the key.