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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fubhy’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, cachedecorator-performance.patch, failed testing.

tim.plunkett’s picture

Title: Improve performance of CacheDecorator and fix ternary return bug. » Improve performance of CacheDecorator
Status: Needs work » Needs review
FileSize
1.12 KB
1.69 KB

Here's some tweaks.

fubhy’s picture

So that's what we call a "tweak" in tim's world folks: REMOVE ALL THE STATIC CACHING!!

tim.plunkett’s picture

Status: Needs review » Needs work

Oh yeah? well yours "failed to clear checkout directory." :P

fubhy’s picture

Status: Needs work » Needs review
FileSize
1.28 KB

Always 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?

yched’s picture

Definite +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.

Fabianx’s picture

Issue tags: +Performance

Tagging appropriately.

isset() in general is very fast as it is no function call, but a build-in operator.

fubhy’s picture

Correct! This issue is rather about reducing the number of method calls.

yched’s picture

As per #7, I'll happily RTBC if we add a code comment :-)

Fabianx’s picture

+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).

fubhy’s picture

Cool. Any volunteers for re-rolling that patch with that docblock? I am on vacation an didn't bring my laptop :/

yched’s picture

OK, will do :-)

yched’s picture

While I was in there, I reshaped the code a bit - purely personal taste, but that's reroller's privilege :-)

yched’s picture

FileSize
1.52 KB

interdiff with #6

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! Lets get that in.

Dries’s picture

Is it worth performance testing this?

xjm’s picture

Yeah, it seems to me if our goal is to improve performance, we should probably have data.

yched’s picture

ab 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.

Fabianx’s picture

I'll do a proper benchmark now :-).

Fabianx’s picture

10 Nodes via /frontpage

=== core..core--1853226--14 compared (50cb8d6c5eaec..50cb8e57a17ae):

ct  : 39,303|39,233|-70|-0.2%
wt  : 160,617|160,162|-455|-0.3%
cpu : 160,010|160,010|0|0.0%
mu  : 12,630,544|12,619,256|-11,288|-0.1%
pmu : 12,818,104|12,807,616|-10,488|-0.1%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

100 Nodes via /frontpage

=== core..core--1853226--14 compared (50cb8f0b11735..50cb8ff9524f9):

ct  : 197,091|196,927|-164|-0.1%
wt  : 737,815|736,563|-1,252|-0.2%
cpu : 732,046|732,046|0|0.0%
mu  : 16,557,568|16,557,232|-336|-0.0%
pmu : 17,257,960|17,259,856|1,896|0.0%

---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage

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!

fubhy’s picture

Note: 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.

fubhy’s picture

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!

Plus 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.

yched’s picture

Just 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().

catch’s picture

Status: Reviewed & tested by the community » Fixed

Yeah 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!

Automatically closed -- issue fixed for 2 weeks with no activity.