Cache option values in BasicGetFieldsAction #34227

ufundo · 2025-12-05T16:45:36Z

Overview

Avoid fetching OptionValues for every row in BasicGetFieldsAction. Improves the performance of getting BasicEntities, with the most notable core example being Afform.

Before

in BasicGetAction, the OptionValues for a field with pseudoconstant.optionGroupName are fetched for every row in order to format pseudoconstants

After

OptionValues are stashed in the cache so they can be reused on subsequent rows

Technical Details

Here's a test script:

git checkout "$1"
cv flush
time cv api4 Afform.get +s confirmation_type:label +s base_module:label > "$1".get1.json
time cv api4 Afform.get +s confirmation_type:label +s base_module:label > "$1".get2.json
time cv api4 Afform.get +s confirmation_type:label +s base_modele:label > "$1".get3.json

3 runs on master:

Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Flushing system caches

real	0m2.154s
user	0m1.877s
sys	0m0.139s

real	0m1.633s
user	0m1.325s
sys	0m0.107s

real	0m1.610s
user	0m1.272s
sys	0m0.116s
Already on 'master'
Your branch is up to date with 'origin/master'.
Flushing system caches

real	0m2.206s
user	0m1.893s
sys	0m0.161s

real	0m1.678s
user	0m1.333s
sys	0m0.112s

real	0m1.540s
user	0m1.246s
sys	0m0.098s
Already on 'master'
Your branch is up to date with 'origin/master'.
Flushing system caches

real	0m2.150s
user	0m1.862s
sys	0m0.148s

real	0m1.637s
user	0m1.308s
sys	0m0.104s

real	0m1.517s
user	0m1.246s
sys	0m0.105s

3 runs on this branch:

Switched to branch 'basic-get-option-caching'
Flushing system caches

real	0m2.116s
user	0m1.866s
sys	0m0.144s

real	0m1.509s
user	0m1.270s
sys	0m0.091s

real	0m1.444s
user	0m1.238s
sys	0m0.109s
Already on 'basic-get-option-caching'
Flushing system caches

real	0m2.205s
user	0m1.902s
sys	0m0.160s

real	0m1.450s
user	0m1.277s
sys	0m0.094s

real	0m1.425s
user	0m1.203s
sys	0m0.104s
Already on 'basic-get-option-caching'
Flushing system caches

real	0m2.128s
user	0m1.865s
sys	0m0.159s

real	0m1.534s
user	0m1.288s
sys	0m0.105s

real	0m1.540s
user	0m1.276s
sys	0m0.107s

So its pretty slight but 5% improvement, on a basic site with 70 afforms. I would expect a lot of sites have more forms, and repeated calls over multiple requests = more savings.

I also have a follow up which adds caching to callbacks, which has more significant performance boost.

Previous caching for pseudoconstant field options seems to only apply to schema/table based fields - this is the exceptional case because it's on tableless entity.

It is a bit odd to me that we dont have a global cache for option values, as far as I can see. There's another CRM_Core_optionValue::getValues(['name' => $name]) type call in SettingsMetadata which might benefit from using the same cache.

civibot · 2025-12-05T16:45:38Z

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

➡️ PR Demo site (can be rebuilt with the command test this please).
📖 Review standards
🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

ufundo · 2025-12-05T16:47:14Z

Civi/Api4/Generic/BasicGetFieldsAction.php

   * @param string $optionGroupName
+   * @deprecated
   */
  public function pseudoconstantOptions(string $optionGroupName) {


This seemed a poorly named function given it only deals with one type of pseudoconstant.

It's also strange as a public instance method on an Api action class.

I didn't see any other callers in core and think it could probably be ripped out, but kept as deprecated for the time being.

@ufundo let's just say mistakes were made when this function was added. Rip out sounds good to me.

It's not called from anywhere in universe. Let's delete it.

ufundo · 2025-12-05T16:48:12Z

@colemanw @eileenmcnaughton I think this could be a good answer to https://lab.civicrm.org/dev/core/-/issues/6229#note_195346

ufundo · 2025-12-05T17:18:38Z

An alternative to this could be to add the caching into \CRM_Core_OptionValue::getValues itself. That looked a bit risky to me because it gets called to check languages, and we put the current locale in the cache key. But could probably be juggled round.

colemanw · 2025-12-05T18:06:47Z

@ufundo Previous caching for pseudoconstant field options seems to only apply to schema/table based fields - this is the exceptional case because it's on tableless entity.

Ahh, well, I was working on that at one point...

Afform - switch entity declaration to standard metadata #31072

If we got that PR over the line, this might become moot.

ufundo · 2025-12-08T08:30:54Z

Ah, that PR looks interesting.

It's always seemed a bit odd to me BasisEntity fields were defined in the getfields action itself. Then again, maybe it's a bit weird to have a schema file... if the general understanding is schema = database schema?

Either way I don't think it makes the whole thing moot, because there other BasicEntities outside of core.

ufundo · 2025-12-08T08:30:54Z

Ah, that PR looks interesting.

It's always seemed a bit odd to me BasisEntity fields were defined in the getfields action itself. Then again, maybe it's a bit weird to have a schema file... if the general understanding is schema = database schema?

Either way I don't think it makes the whole thing moot, because there other BasicEntities outside of core.

ufundo · 2025-12-08T11:02:09Z

It would definitely be nice for BasicEntities to go through EntityMetadataBase::getOptions.

I don't know my way round all of the EntityRepository stuff, but is the PR using SQLEntityMetadata for the Afform? It feels like table-less entities should have a separate subclass of EntityMetadataBase.

I think it could be nice to store the metadata for tableless entities somewhere else also - maybe directly on the \Civi\Api4[Entity] class, as public const? Then you could create the Api-only entity with a single file. It doesn't need to be picked up by stuff that does schema manipulation

ufundo · 2025-12-08T18:05:11Z

Do you have any reservations with this version for now @colemanw ?

Civi/Api4/Generic/BasicGetFieldsAction.php

Ensures option lists are cached for non-DAO entities like Afform Fixes dev/core#6229

…atic methods

civibot bot added the master label Dec 5, 2025

ufundo commented Dec 5, 2025

View reviewed changes

ufundo marked this pull request as ready for review December 5, 2025 16:48

ufundo mentioned this pull request Dec 5, 2025

Cache callback options in BasicGetFieldsAction #34228

Open

ufundo mentioned this pull request Dec 6, 2025

Api4 - Optimize option lookup in getFields #34222

Closed

colemanw reviewed Dec 8, 2025

View reviewed changes

Civi/Api4/Generic/BasicGetFieldsAction.php Outdated Show resolved Hide resolved

ufundo force-pushed the basic-get-option-caching branch 2 times, most recently from 4bc14b0 to 7824a12 Compare December 8, 2025 18:36

colemanw and others added 4 commits December 8, 2025 18:38

Api4 - Optimize option lookup in getFields

3d4aec1

Ensures option lists are cached for non-DAO entities like Afform Fixes dev/core#6229

[NFC] FormattingUtil - use self rather than absolute reference for st…

0994c99

…atic methods

[REF] BasicGetFieldsAction - factor out getting pseudoconstant options

e21a3b6

BasicGetFieldsAction - add caching for option groups

bfa8fae

ufundo force-pushed the basic-get-option-caching branch from 7824a12 to bfa8fae Compare December 8, 2025 18:38

Api4 - Add ORDER BY to fetchOptionValues

40380de

colemanw merged commit 9b0106a into civicrm:master Dec 8, 2025
1 check passed

Uh oh!

Cache option values in BasicGetFieldsAction #34227

Cache option values in BasicGetFieldsAction #34227

Conversation

ufundo commented Dec 5, 2025

Overview

Before

After

Technical Details

Uh oh!

civibot bot commented Dec 5, 2025

Uh oh!

ufundo Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

colemanw Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

colemanw Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

ufundo commented Dec 5, 2025

Uh oh!

ufundo commented Dec 5, 2025

Uh oh!

colemanw commented Dec 5, 2025

Uh oh!

ufundo commented Dec 8, 2025

Uh oh!

ufundo commented Dec 8, 2025

Uh oh!

ufundo commented Dec 8, 2025

Uh oh!

ufundo commented Dec 8, 2025

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants