Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We should try to reduce the number of method calls in the CacheDecorator to a minimum. Also, it currently trigger the ternary return bug as described here: #1838368: Do not blindly use the ternary operator; it always returns copies of non-object values.
Comment | File | Size | Author |
---|---|---|---|
#15 | interdiff.txt | 1.52 KB | yched |
#14 | cacheDecorator_optim-1853226-13.patch | 1.57 KB | yched |
#6 | 1853226-6.patch | 1.28 KB | fubhy |
#3 | cachedecorator-1853226-1.patch | 1.69 KB | tim.plunkett |
#3 | interdiff.txt | 1.12 KB | tim.plunkett |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedComment #3
tim.plunkettHere's some tweaks.
Comment #4
fubhy CreditAttribution: fubhy commentedSo that's what we call a "tweak" in tim's world folks: REMOVE ALL THE STATIC CACHING!!
Comment #5
tim.plunkettOh yeah? well yours "failed to clear checkout directory." :P
Comment #6
fubhy CreditAttribution: fubhy commentedAlways a pleasure to work with you, tim :P.
Btw. ... I find it kinda weird that we check for isset($this->cacheKey) all the time. Was that an attempt to make persistent caching optional? I am wondering: Do we wan't to make that optional? Does that even make sense? Is there a case where we want to statically cache plugin definitions and not cache them in the persistent cache?
Comment #7
yched CreditAttribution: yched commentedDefinite +1 on reducing the # of func calls to access definitions once the static cache is warm.
Only nitpick : IMO this would deserve a code comment about why the code is made "a bit less straightforward than it could" - just like our current uses of the "static $drupal_static_fast" pattern are usually explicitly commented.
Something like
// Optimize for fast access if the definitions are already in memory.
?Regarding
isset($this->cacheKey)
: yeah, I was puzzled by this myself, and assumed it's a non-documented feature of "CacheDecorator can cache statically without caching persistently". Whether this really makes sense is a separate discussion IMO. Those isset checks shouldn't have a perf impact.Comment #8
Fabianx CreditAttribution: Fabianx commentedTagging appropriately.
isset() in general is very fast as it is no function call, but a build-in operator.
Comment #9
fubhy CreditAttribution: fubhy commentedCorrect! This issue is rather about reducing the number of method calls.
Comment #10
yched CreditAttribution: yched commentedAs per #7, I'll happily RTBC if we add a code comment :-)
Comment #11
Fabianx CreditAttribution: Fabianx commented+1 for #6with code comment, then RTBC. Straightforward change.
That said is whatever is clearing the cache also affecting $this->definitions? (I guess so, but just making sure the static cache can be cleared).
Comment #12
fubhy CreditAttribution: fubhy commentedCool. Any volunteers for re-rolling that patch with that docblock? I am on vacation an didn't bring my laptop :/
Comment #13
yched CreditAttribution: yched commentedOK, will do :-)
Comment #14
yched CreditAttribution: yched commentedWhile I was in there, I reshaped the code a bit - purely personal taste, but that's reroller's privilege :-)
Comment #15
yched CreditAttribution: yched commentedinterdiff with #6
Comment #16
Fabianx CreditAttribution: Fabianx commentedLooks good! Lets get that in.
Comment #17
Dries CreditAttribution: Dries commentedIs it worth performance testing this?
Comment #18
xjmYeah, it seems to me if our goal is to improve performance, we should probably have data.
Comment #19
yched CreditAttribution: yched commentedab run on a view listing 10 nodes, 10 fields each, 2 filters, 1 sort
8.x: 151,794 ms
patch: 150,482 ms -> -0,86%
Not dramatic, but given that more and more things are targetted for being plugins eventually, making sure that the access to a plugin definition is not at least 3 function calls away with warm static caches sounds reasonable IMO. EntityNG makes a heavy use of accessing plugin definitions.
Comment #20
Fabianx CreditAttribution: Fabianx commentedI'll do a proper benchmark now :-).
Comment #21
Fabianx CreditAttribution: Fabianx commented10 Nodes via /frontpage
100 Nodes via /frontpage
It is marginal, but it saves some function calls, so I think getting it in is still good.
Note: The CacheDecorator::getDefinition is just called 91 times for the 100 nodes example and 44 times for the 10 nodes example. So with SCOTCH this will increase a lot!
Comment #22
fubhy CreditAttribution: fubhy commentedNote: reducing the number of function calls is just one of the gains here. The other main goal is to work around the ternary return bug as described in the issue summary. This is important as shown in the comment entityng conversion issue.
Comment #23
fubhy CreditAttribution: fubhy commentedPlus that.. Yes. To clarify: Once we start using the CacheDecorator more extensively (which we most certainly will be doing) the performance gain will increase too.
Comment #24
yched CreditAttribution: yched commentedJust to summarize : I don't expect to be able to prove consequent perf gains on current HEAD with this patch.
The point is that
- Plugins are going to be all over the place in D8 core + contrib. That's why Plugin API was built for. We haven't even converted all the core subsystems that would apply, and we're already at more than 20 plugin types.
- Plugin objects can access their own definitions via PluginBase::getDefinition() (which internally ends up calling $discovery->getDefinition()).
The number of calls on a given page can be arbitrarily high, depending on the plugins involved. According to @fago, EntityNG and TypeData make a heavy use of that - in #1778178: Convert comments to the new Entity Field API he even considered duplicating the data in separate static caches to make them faster to get to (see #54 #55 over there).
Thus, CacheDecorator::getDefinition[s]() qualifies as part of the critical path, and reducing access to the data from 3 function calls to 1 function call at the cost of *minimally* less pure code (we're talking about CacheDecorator, that is like 15 lines of methods code), is totally legit IMO. Same as we have a "drupal_static_fast" construct for drupal_static().
Comment #25
catchYeah this is a very small change. If it ends up being marginal for performance we didn't lose much anyway. Committed/pushed to 8.x. Thanks!