SearchPluginManager needs a $this->setCacheBackend() in the constructor as other have. Otherwise, the annotations are re-parsed on every request.

Files: 
CommentFileSizeAuthor
#12 search-plugin-definitions-cached.patch1.7 KBBladedu
PASSED: [[SimpleTest]]: [MySQL] 59,177 pass(es).
[ View ]
#5 search-plugin-definitions-cached.patch1.69 KBBladedu
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]
#5 interdiff.txt581 bytesBladedu
#4 search-plugin-definitions-cached.patch1.42 KBBladedu
FAILED: [[SimpleTest]]: [MySQL] 58,176 pass(es), 47 fail(s), and 1,585 exception(s).
[ View ]

Comments

Issue tags:+Performance

Tagging.

Yes, I noticed this too while rolling #2087965: The SearchPluginManager (maybe?) needs to provide an alter hook for its discovery. Since the 2 patches would conflict, maybe let's finish that 1st.

Assigned:Unassigned» Bladedu

Status:Active» Needs review
StatusFileSize
new1.42 KB
FAILED: [[SimpleTest]]: [MySQL] 58,176 pass(es), 47 fail(s), and 1,585 exception(s).
[ View ]

This patch should resolve the issue.

StatusFileSize
new581 bytes
new1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]

Forgot to add the namespaces of CacheBackendInterface and LanguageManager. This should be fine.

I took a look at what some other plugin managers are doing... Example from BlockManager:

  $this->setCacheBackend($cache_backend, $language_manager, 'block_plugins');

So maybe we should be saying 'search_plugins' in the call here, rather than just 'search'?

I'm also a bit confused about what's correct for the services.yml file. Block says '@cache.block' and then it sets up its own cache. Do we need to do that here as well? I'm a bit ignorant on all of this...

You happened to look at almost the only one that does use a specific bin. Using the default one is fine, cache key also IMHO also fine, as long as it's unique...

Note that this will conflict with the issue mentioned in #2, but I think this one is a pretty severe Performance regression, so IMHO more important. I'd also be just fine with merging them into a single issue.

I think I agree with jhodgdon that the cache key prefix should be a little more specific - otherwise it looks ready.

Status:Needs review» Needs work

Ok, let's go for search_plugins then?

It's fine by me to go for search_plugin, but I propose that we also fix the other plugins to be consistent with the naming convention. I can open a new issue for that and provide a patch for it.

Maybe, feel free to open an issue, but such issues often result in discussions, because it's not the same. Renaming for example the entity manager cache id would be weird ;)

Let's just fix this and get this in, I just want to have the caching fixed asap :)

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
PASSED: [[SimpleTest]]: [MySQL] 59,177 pass(es).
[ View ]

Renamed the cache key prefix to 'search_plugins'. I'll create another issue and I'll propose another patch for the naming conventions of other cache key prefixes.

Status:Needs review» Reviewed & tested by the community

Ok, looks good to me, let's do this :)

Thanks for the explanations Berdir, and the patching Bladedu. Looks good to me too!

This never gets cleared anywhere. Should it be, or is that only going to happen on module install/uninstall anyway?

I think that's fine, many plugin managers work like that. A call to clearCachedDefinitions() would take care of it, and it's not like you're changing them "frequently" like views plugins.

Status:Reviewed & tested by the community» Fixed

OK works for me.

Committed/pushed to 8.x, thanks!

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