refactor(page_service / timeline::handle): the GateGuard need not be a special case #11030
+131
−108
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
…Open stuff into separate struct
…reference cycles Storing Arc<Timeline> inside RequestContext risks reference cycles if the RequestContext gets stored in an object that is owned directly or indirectly by Timeline. So, we wrap the TimelineMetrics into an Arc and propagate that instead. This makes it easy for future metrics to be access through RequestContext, like we do for storage_io_size here. To make the page_service case happy, where we may be dispatching to a different shard every successive request, and don't want to be cloning from the shared Timeline::metrics on each request, we pre-clone as part of the handle cache miss.
problame
requested review from
erikgrinaker,
VladLazar and
arpad-m
and removed request for
erikgrinaker
7744 tests run: 7365 passed, 0 failed, 379 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
823bf12 at 2025-02-28T12:39:51.294Z :recycle: |
VladLazar
approved these changes
Feb 28, 2025
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.
Changes
While working on
I found myself needing to cache another expensive Arc::clone inside
inside the timeline::handle::Cache by wrapping it in another Arc.
Before this PR, it seemed like the only expensive thing we were caching was the
connection handler tasks' clone of
Arc<Timeline>.But in fact the GateGuard was another such thing, but it was special-cased
in the implementation.
So, this refactoring PR de-special-cases the GateGuard.
Performance
With this PR we are doing strictly less operations per
Cache::get.The reason is that we wrap the entire
Types::Timelineinto one Arc.Before this PR, it was a separate Arc around the Arc and
one around the Arc.
With this PR, we avoid an allocation per cached item, namely,
the separate Arc around the GateGuard.
This PR does not change the amount of shared mutable state.
So, all in all, it should be a net positive, albeit probably not noticable
with our small non-NUMA instances and generally high CPU usage per request.
Reviewing
To understand the refactoring logistics, look at the changes to the unit test types first.
Then read the improved module doc comment.
Then the remaining changes.
In the future, we could rename things to be even more generic.
For example,
Types::TenantMgrcould really be aTypes::Resolver.And
Types::Timelineshould, to avoid constant confusion in the doc comment,be called
Types::CachedorTypes::Resolved.Because the
handlemodule, after this PR, really doesn't care that we'reusing it for storing Arc's and GateGuards.
Then again, specicifity is sometimes more useful than being generic.
And writing the module doc comment in a totally generic way would
probably also be more confusing than helpful.