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.

Files: 
CommentFileSizeAuthor
#15 interdiff.txt1.52 KByched
#14 cacheDecorator_optim-1853226-13.patch1.57 KByched
PASSED: [[SimpleTest]]: [MySQL] 49,343 pass(es).
[ View ]
#6 1853226-6.patch1.28 KBfubhy
PASSED: [[SimpleTest]]: [MySQL] 48,829 pass(es).
[ View ]
#3 cachedecorator-1853226-1.patch1.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 48,785 pass(es).
[ View ]
#3 interdiff.txt1.12 KBtim.plunkett
cachedecorator-performance.patch1.23 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

Comments

Status:Active» Needs review

Status:Needs review» Needs work

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

Title:Improve performance of CacheDecorator and fix ternary return bug.Improve performance of CacheDecorator
Status:Needs work» Needs review
StatusFileSize
new1.12 KB
new1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 48,785 pass(es).
[ View ]

Here's some tweaks.

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new1.28 KB
PASSED: [[SimpleTest]]: [MySQL] 48,829 pass(es).
[ View ]

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?

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.

Issue tags:+Performance

Tagging appropriately.

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

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

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

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

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

OK, will do :-)

StatusFileSize
new1.57 KB
PASSED: [[SimpleTest]]: [MySQL] 49,343 pass(es).
[ View ]

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

StatusFileSize
new1.52 KB

interdiff with #6

Status:Needs review» Reviewed & tested by the community

Looks good! Lets get that in.

Is it worth performance testing this?

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

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.

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

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!

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.

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.

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

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.