APIv4 - RequestSpec caching #29066
APIv4 - RequestSpec caching #29066
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
colemanw
force-pushed
the
requestSpec
branch
3 times, most recently
from
3813b7f to
08df917
Compare
|
@colemanw I feel good about test cover here but can you explain the & secondary to that - do we need a docs update ? |
|
@eileenmcnaughton this PR basically puts a cache in front of every I'll expand the PR description a bit. Docs update might be in order too, I'll check... |
colemanw
force-pushed
the
requestSpec
branch
2 times, most recently
from
00bcb3e to
f0ddcad
Compare
Optimization is not very helpful now that custom groups and fields are cached. Getting rid of it opens the door to a better optimization for caching the entire spec.
This was only being used by the ActionMapping classes and we want to avoid it so values are always fetched with a getter rather than from a massive array.
This is an improved version of getFieldValue() that uses late-static binding and returns formatted values.
This adds in-memory caching to speed up cases where getFields is called hundreds of times per page. Because the array of $values can be literally anything, we cut it down to just the relevant ones by tracking which values are actually used by the specProviders.
APIv4 now caches the output of all SpecProviders, so there is no need for them to use internal caching.
|
@eileenmcnaughton I've rebased this now that #29072 is merged. This is ready as far as I'm concerned. |
|
timing 5.69
master with other recent patches, but not this one
master with this patch too
So this shaved a further 15 seconds-ish off the time the page took to load. That's a definite win - although even 18.72 seconds still feels like there is quite a bit of room for improvement. I may yet do another cache grind to see but this seems like a clear improvement and our cumulative improvement is dramatic |
|
@colemanw I may have been too hasty here - the fields seem to no longer be editable! Still probably easier to address as a follow up since this moves a lot of stuff |
|
@eileenmcnaughton hmm, I thought we had tests for that. Quick test shows the fields as editable for me. Can you give a example search? |
|
OK - update - the editable was broken without this but works in 5.68 - but is broken in 5.69. Importantly the import search kits rely on a non-standard primary key which could be what broke? |
|
@colemanw hang on - it might be that they are not editble because the rows imported - let me try changing the status to error & see |
|
Yep - that's it! All good |
Overview
Implements in-memory caching for APIv4 getFields. This significantly speeds up SearchKit, especially when the display includes links or in-place editable fields.
Before
Every call to getFields would rebuild
FieldSpecobjects from scratch and call everySpecProviderfor modifications, then transform those objects into an array & filter it down to maybe just the one field you wanted in the first place.After
All of that is cached.
Technical Details
Caching happens as late as I could manage it... after the
FieldSpecobjects have been modified by everySpecProviderand converted to their final array form, but before pseudoconstant options are loaded (there's already a cache for those somewhere, I think).Getting a cache key for this was tricky since
getFieldsaccepts an array of$valueswhich could contain possibly anything. I solved this by tracking which values are actually used by the specProviders. Every time a specProvider callsRequestSpec->getValue()we keep track of that and it gets added to the cacheKey.Probably 90% of entities don't have any fancy specProviders so our tracker knows that all $values can be safely ignored & this keeps caching for them very simple. For more complex entities, there will be considerably more copies of the fields in memory; hopefully that won't be a problem.
Civi::cache('metadata')->clear()will trigger a flush of this in-memory cache as well, although that dispatcher was broken so I had to fix it via #29072.Breaking Changes
The changes in this PR are mostly internal. While a couple of them could hypothetically affect extensions that have very deep integrations with APIv4, a
universesearch shows that to be mostly hypothetical.FieldSpec::setOptionsCallbackfunction now gives$fieldas an array rather than an object. This is a pretty obscure feature outside of core. I was only able to find one such example inuniverse(and it was written by me). I submitted a PR to update that callback.SpecProviderInterface::appliesno longer receives$valuesas a 3rd param. This was only added recently and never documented so I can't find anything inuniverseoutside of core, nor would I expect to.