wpcomsh: Optimize Atomic_Storage_Provider with per-request caching and reduced DB queries#48023
wpcomsh: Optimize Atomic_Storage_Provider with per-request caching and reduced DB queries#48023bindlegirl wants to merge 2 commits intotrunkfrom
Conversation
Made-with: Cursor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
fgiannar
left a comment
fgiannar
left a comment
There was a problem hiding this comment.
Thanks for working on these improvements, Ana!
I'm testing on my WoA dev site with the Broken Token functionality so I can actually see the user tokens. Interestingly, while I do my user token on trunk, it not present on this branch. I do have a hypothesis, curious if it will fix my issue:
In resolve_user_by_email the cache negatively memoizes failed lookups, and any subsequent call with the same email gets the cached null without retrying.
Here's the exact path:
private function resolve_user_by_email( $email ) {
if ( $email === $this->resolved_email ) {
return $this->resolved_user; // ← can return cached NULL
}
$this->resolved_email = $email; // ← set to a valid email
$this->resolved_user = null; // ← reset to null
if ( ! function_exists( 'get_user_by' ) || empty( $email ) || ! is_email( $email ) ) {
return null; // ← bails, leaves resolved_user=null
} // but resolved_email=$email is now set
$user = get_user_by( 'email', $email );
if ( $user instanceof \WP_User ) {
$this->resolved_user = $user;
} // ← if get_user_by returns false,
return $this->resolved_user; // resolved_user stays null,
} // resolved_email is the real email
So any of these will poison the cache:
- Pluggable not loaded yet on the first call: function_exists( 'get_user_by' ) is false → returns null → resolved_email is set to the real email → next call with the same email returns cached null even though
pluggable is loaded now. - get_user_by returns false on the first call (user not yet present in the local DB on a replicated site, transient miss, etc.) → caches null → second call gets the cached null even though the user is now
resolvable. - Order-of-operations within get(): provider->get('master_user') runs before provider->get('user_tokens'). If the master_user path hits the failure mode above, the user_tokens path sees the poisoned cache and
get_user_tokens() returns false at if ( ! $user ) → you get empty tokens.
On trunk, neither call caches anything, so each invocation does its own fresh get_user_by() lookup and the second one succeeds even if the first one failed.
I believe we can fix this by not caching negative results. WP's own user-cache layer makes repeated get_user_by('email', …) calls cheap anyway, so the dedupe value of the per-instance cache is really only for the positive hit.
private function resolve_user_by_email( $email ) {
// Only serve from cache when we have a positive resolution.
if ( null !== $this->resolved_user && $email === $this->resolved_email ) {
return $this->resolved_user;
}
$this->resolved_email = $email;
$this->resolved_user = null;
if ( ! function_exists( 'get_user_by' ) || empty( $email ) || ! is_email( $email ) ) {
return null;
}
$user = get_user_by( 'email', $email );
if ( $user instanceof \WP_User ) {
$this->resolved_user = $user;
}
return $this->resolved_user;
}
It would be nice if we could also add a unit test for this case :)
| /** | ||
| * Test that re-entrancy flag is reset even if get_option throws. | ||
| */ | ||
| public function test_reentrant_flag_reset_on_exception() { |
There was a problem hiding this comment.
How do we trigger the exception here?
2 participants
Proposed changes
Four performance optimizations for the
Atomic_Storage_Providerto reduce per-request DB queries and redundant work:Atomic_Persistent_Datainstance across allget()calls instead of creating 4 per request (one per option). APD data is immutable within a request.resolve_user_by_email()that caches theWP_Userlookup. Bothget_master_user_id()andget_user_tokens()use the same owner email, so this guarantees at most 1 DB lookup regardless of object-cache state.get_raw_option()(direct DB query) withJetpack_Options::get_option()(reads from WP's autoloaded options cache) when loading existing tokens. A re-entrancy flag onshould_handle('user_tokens')prevents the circular dependency. Wrapped intry/finallyfor safety. This is the same pattern used by CIAB/Garden.validate_user_tokens()re-reads the latest DB state and finds no remaining conflicts (already resolved by a concurrent request), skip the write + delete + cache flush.Impact summary:
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
AtomicStorageProviderTesttests cover all code paths (conflict resolution, orphaned token cleanup, matching tokens, invalid inputs). 5 new tests added for the caching and re-entrancy behaviors.jp docker phpunit wpcomsh -- --filter=AtomicStorageProviderto execute the test suite.