FIX Opt-in performance fix for many consecutive lookups using isLocalisedInStage #468
Conversation
|
Let's implement silverstripe/silverstripe-framework#8391 and then refactor this PR to provide a |
|
This should probably be committed to master / 4.2.x-dev |
|
A patch for this is here https://gist.github.com/bec52b2d0f50a42f44a0aea2a05bc2af If you can give me commit rights to the creative-commoners repo I'll push it to this branch |
|
@sminnee done |
| return; | ||
| } | ||
|
|
||
| // This functionality hasn't been implemented; it's not actually used right now, so assumeYAGNI |
There was a problem hiding this comment.
Can we clarify this comment a bit? The end doesn't make sense to me
ScopeyNZ
force-pushed
the
pulls/4.1/affluent-caches
branch
from
01b2e3a to
c724a85
Compare
|
I have updated the comment. I just translated the acronym 🙂 |
|
Ok so is this PR dependant on silverstripe/silverstripe-framework#8380? If so, we'll need to prepare a new minor release which has a 4.3 minimum in core, since that PR was merged into 4.3.x-dev |
|
Right. I can rebase this onto |
|
@robbieaverill @ScopeyNZ this doesn't require 4.3+, but it's a no-op until 4.3 is installed. I don't think the constraint change is required then? |
|
OK sweet |
|
In that case there's no rebase required. It'd be cool to get Travis going before merge (I've pinged Damian), but if they pass locally then go for it |
|
I wonder if @tractorcow wants to invite some commoners to help till the farm? |
|
Probably need to write some unit test for this as well. |
ScopeyNZ
force-pushed
the
pulls/4.1/affluent-caches
branch
from
5e91803 to
46c53a2
Compare
ScopeyNZ
force-pushed
the
pulls/4.1/affluent-caches
branch
from
46c53a2 to
77a3a6f
Compare
| @@ -304,29 +332,99 @@ protected function isLocalisedInStage($stage, $locale = null) | |||
| $table .= self::SUFFIX_LIVE; | |||
| } | |||
|
|
|||
| if (isset(static::$idsInLocaleCache[$locale][$table])) { | |||
There was a problem hiding this comment.
I feel like this isset check should include the owner's ID like the line below
There was a problem hiding this comment.
The reason I didn't is because we know all the IDs that exist in the locale for that table - the absence of an ID implies it's not there. So items that aren't in that stage/locale will skip this cache if you isset check the ID too.
There was a problem hiding this comment.
I just checked this. Omitting owner ID drops another 105 queries... I'll PR to remove it again and we can continue discussion there 🙂
… tests for static caching
|
I've done a little bit of a refactor in order to unit test this easily, and have pushed some new tests. LGTM. I think @tractorcow's feedback has been addressed |
robbieaverill
dismissed
tractorcow’s stale review
I think these comments have been addressed
|
We don't have Travis set up on this org yet (cc @tractorcow) @tractorcow if you don't think we've correctly addressed your comments feel free to ping us and we'll look more into it =) |
This PR adds a static method to pre-populate a cache of object IDs that exist in the given locale. This can be opted into by calling this method from appropriate user-code (or user-code extension) or alternatively by providing a list of classes that should be pre-populated (done when the first call of
isLocalisedInStageis performed for that class).Ideally I wouldn't have this config option but it is currently quite difficult to find a location to prepopulate this cache when running the SiteTree, although I'm open to suggestions here and more than happy to remove the configuration part of this PR.
Fixes #467