Pages with multiple instances on CKEditor 5 are unnecessary slow

This needs to be fixed on 11.x (our main development branch) first, and are then back ported as needed according to our policies. Also, 10.2 is in security mode now.

This makes sense. I have a customer who leverages paragraphs for their site, often with a dozen or more rich text fields on given node edit page. Page load times are quite slow in these instances.

The ::getAttachments() method belongs to the Drupal\editor\Plugin\EditorManager class, so moving this issue to editor.module.

Static caching seems reasonable. &drupal_static() can be used. Editors and text formats have a 1:1 relationship. Text formats have permissions and control access their editor, so I don't see any permission issue here with caching.

We'll want to make sure to preserve the behavior of hook_editor_js_settings_alter() (i.e. $this->moduleHandler->alter('editor_js_settings', $settings); and below should not be cached.

Put up an MR with changes to statically cache the editor attachments.

We'll want to make sure to preserve the behavior of hook_editor_js_settings_alter() (i.e. $this->moduleHandler->alter('editor_js_settings', $settings); and below should not be cached.

This could an issue if people are doing expensive operations in their alter hook implementations. The alter hook does not provide a way to identify editors by instance, so off the top of my head, I can't think of a reason not to cache after the settings have been altered. That being said, the MR caches before the alter hook, partly because it would break the existing automated tests by caching after.

Moving to "Needs work" because I think there are tests that need to be added, but I'm not sure what kind of tests.

@godotislate - I like the approach of caching attachments in a class property. One thing I noticed when stepping through the method with a debugger is that editor_load() still gets called every loop if the text format doesn't have an editor assigned to it.

I just pushed a commit to the MR that add an $editors property to the class and checks if the text format has been processed yet instead of checking if $settings['editor']['formats'][$format_id] is set. This ensures editor_load() is only called once for each text format.

I'm not sure what kind of text coverage could be added for this MR. We can't test performance with unit tests, and the existing test coverage should ensure we haven't broken anything.

Issue summary appears incomplete.

This also seems like something that probably should have test coverage but not sure what that would look like.

Updated issue summary.

I don't see a way to "add" test coverage for this performance optimization. Existing test coverage should ensure we haven't broken anything and should cover the changes made by the MR. As of last run, tests are passing.

We actually have performance testing in core.

I'd missed that addition. Here's a link to the D.O. documentation: https://www.drupal.org/docs/develop/automated-testing/performance-tests → .

It looks like we'd probably use the getQueries() method and/or the ->getQueryCount() method on the PerformanceData class.

I did post into slack asking for a 2nd opinion

Okay got a 2nd opinion from @catch in slack. It would be nice to have performance coverage for ckeditor but most likely out of scope of this particular issue. So I opened 📌 Write performance test for ckeditors Active for adding that.

So believe this one is good to go

So this fixes the problem for this case, but I think we should consider changing editor_load() instead (or maybe as well).

editor_load() attempts to pre-load all editor config with a multiple config entity load, this is to take advantage of multiple loading - either a cache getMultiple() or from storage.

However, when caches are warm, this is exchanging multiple cache gets (which as it's config will be against apcu) for a database query + a single cache get - this happens in ConfigEntityStorage::loadMultiple() which calls ConfigFactory::listAll().

If we only load the editors that are actually requested, we don't need the listing query.

If we only load the editors that are actually requested, we don't need the listing query.

In that case, would there be any reason not to just deprecate editor_load($format_id)? It would be no different from Editor::load($format_id)/$this->entityTypeManager()->getStorage('editor')->load($format_id).

Refactored this a little to use Editor::loadMultiple() with a set of IDs passed in, instead of editor_load(). This way there is only one query to load the editor entities, but also leverages the entity cache and does not call listAll().

One possible issue that came to mind is that it is possible that editor entity CRUD actions during a request, so the statically cached copies of attachments and editors in the EditorManager would be stale. This doesn't seem to be all that likely, except possibly in tests, but this doesn't seem to affect existing tests since they all pass. It's possible to bring in memory cache-like functionality, but that increases complexity.

If we make the editor config entity type statically cacheable (if it's not already), would we need the dedicated static cache here at all?

I thought we'd already done that for all config entities but #1885830: Enable static caching for config entities → is still open, however individual config entities can opt in.

If we make the editor config entity type statically cacheable (if it's not already), would we need the dedicated static cache here at all?
I believe the editor config entity type is statically cached in via entity storage, but I think the weird issue here is that there are formats that could possibly not use an editor, so we'd have to track format IDs that an attempt to editors for but don't exist.

Even without statically caching the editor entities here, I think we'd still want to statically cache the attachments/settings.

I believe the editor config entity type is statically cached

Actually, looks like no, it's not. Only Role is opted in?

Made the change per #17 to editor_load() and deprecated after conversation on Slack. Added a CR.

Feedback appears to have been addressed.

One comment on the MR.

Addressed comment.

The comment was straightforward and addressed.

I've been following and reviewing this issue for a bit so I agree rtbc.

Committed/pushed to 11.x, thanks!

catch → closed merge request !9609

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