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

Berdir’s picture

Issue tags:+Performance

Tagging.

pwolanin’s picture

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

Bladedu’s picture

Assigned:Unassigned» Bladedu
Bladedu’s picture

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.

Bladedu’s picture

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.

jhodgdon’s picture

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

Berdir’s picture

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.

pwolanin’s picture

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

Berdir’s picture

Status:Needs review» Needs work

Ok, let's go for search_plugins then?

Bladedu’s picture

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.

Berdir’s picture

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 :)

Bladedu’s picture

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.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

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

jhodgdon’s picture

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

catch’s picture

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

Berdir’s picture

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.

catch’s picture

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.