Noticed in #3518179: Renderer::executeInRenderContext() needs to handle nested calls and suspended fibers but it turns out to be unrelated to that issue.
On a cold cache (tested with Umami), _ckeditor5_get_langcode_mapping() gets called multiple times, it has a persistent cache but not a static cache, meaning it makes potentially dozens of round trips to the cache backend that could be one if there are lots of big pipe placeholders.
We could move this to a service and then keep the mappings on a class property, then only fetch them from the cache once.
While individual cache gets aren't expensive, we do 472 cache gets on the Umami front page with a cold cache, and if each cache get is 0.3ms, that would take over 150ms.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !13694
Comments
Comment #2
catchComment #3
berdirNote: 25 of the 45 default cache bin lookups in \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testNodePageAdministrator() are this. So we could save 24 of those in that scenario.
Always a bit awkward with those internal functions, which are then actually used in completely different places (hooks and the ckeditor plugin), we can't just inline it into the hooks class, we need to make it a service, maybe tagged with @internal. We could make it a private service, then it would be a separate object instance, with the shared memory cache instance.
We could also put this in discovery (probably additionally, 25 lookups to apcu with unserialize still costs us _something_), because it's tiny, frequently used and a single, stable cache ID.
Comment #5
berdirWe could also do this without the OOP conversion, but we have to do this anyway, so why not together.
It's an internal function, no known calls, so I don't think we need a change record, kept the old function with a reference to this MR. Can't make it private due to the BC layer.
performance tests need to be fixed.
Comment #6
berdirUpdated the performance tests.
Comment #7
catchThis looks great, can't see anything else to complain about. We can convert the one constructor to property promotion separately.
Easy to forget this happens on every cold cache request regardless of whether ckeditor appears on the page or not - because it's called when building library definitions which is global.
Comment #8
nod_Committed and pushed 2592e8ba422 to 11.x and ea0226fd568 to 11.3.x. Thanks!