Scope: minimize the number of gets #41
Conversation
If getting the scope prefix value is a miss, then we all keys under the scope are by definition a miss, so we don't need to bother looking. This defers the generating and storing of the scope prefix value till we actually need it (on a set). In practice, this will reduce the number of gets if using getMultiple() or if you don't always *set()* on a miss.
danielbeardsley
force-pushed
the
scope--minimize-storing
branch
from
66bfe5f to
0621920
Compare
There was a problem hiding this comment.
Pull request overview
This PR optimizes cache operations in the Scope class by deferring scope prefix generation until needed. When a scope prefix doesn't exist in cache, all keys under that scope are automatically misses, so the optimization short-circuits unnecessary cache lookups.
Key changes:
- Modified
getScopePrefix()to accept agenerateOnMissparameter that defers prefix generation - Added overridden
get()andgetMultiple()methods that short-circuit when scope prefix is missing - Updated GitHub Actions workflow to use newer action versions (v2→v4)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| library/iFixit/Matryoshka/Scope.php | Implements deferred scope prefix generation and short-circuit logic for get operations |
| tests/ScopeTest.php | Adds test coverage for the short-circuit behavior in both get() and getMultiple() |
| .github/workflows/php.yml | Updates GitHub Actions to newer versions (maintenance update) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
masonmcelvain
left a comment
masonmcelvain
left a comment
There was a problem hiding this comment.
CR 👏🏻 but dev_block 👍🏻 on what might be a bug
library/iFixit/Matryoshka/Scope.php
Outdated
| } | ||
| if ($reset || ($scopeValue === self::MISS && $generateOnMiss)) { | ||
| $scopeValue = substr(md5(microtime() . $this->scopeName), 0, 16); | ||
| $this->backend->set($this->getScopeKey(), $this->scopePrefix); |
There was a problem hiding this comment.
Isn't $this->scopePrefix still null at this point? It seems like we want to point the scopeKey at the scopeValue
| $this->backend->set($this->getScopeKey(), $this->scopePrefix); | |
| $this->backend->set($this->getScopeKey(), $scopeValue); |
There was a problem hiding this comment.
Ooof.. This was way off. Thanks. I refactored it and added a test.
The Short circuit should prevent getMultiple from being called and we weren't explicitly check for that.
We weren't using the $scopeValue we just created when doing the set(). Also, refactor it for clarity. Add a test to ensure we're setting the value.
masonmcelvain
left a comment
masonmcelvain
left a comment
There was a problem hiding this comment.
CR 👏🏻 Looks much better, thanks!
| } | ||
| } else { | ||
| $this->scopePrefix = "{$scopeValue}-"; | ||
| } |
There was a problem hiding this comment.
Minor: I think we could de-duplicate the $this->scopePrefix = "{$scopeValue}-"; assignment here
| } | |
| if ($scopeValue === self::MISS) { | |
| if ($generateOnMiss) { | |
| $scopeValue = substr(md5(microtime() . $this->scopeName), 0, 16); | |
| $this->backend->set($this->getScopeKey(), $scopeValue); | |
| } else { | |
| return self::MISS; | |
| } | |
| } | |
| $this->scopePrefix = "{$scopeValue}-"; |
If getting the scope prefix value is a miss, then we all keys under the scope are by definition a miss, so we don't need to bother looking.
This defers the generating and storing of the scope prefix value till we actually need it (on a set). In practice, this will reduce the number of gets if using getMultiple() or if you don't always set() on a miss.