We currently cache field data for each object by id and version id.
- It adds no real performance benefit in 99% use cases
- It potentially makes cache space explode, which might make memcache-based caching less efficient
- It makes the 'clear stale field cache' strategy for 'reference fields' (noderef field, filefield, taxo as field), as defined in #111127-207: Cache node_load much less efficient

CommentFileSizeAuthor
#11 field_no_revision_cache-456488-10.patch4.09 KByched
#9 field_no_revision_cache-456488-8.patch14.56 KByched
#8 field_no_revision_cache-456488-8.patch14.56 KByched
#6 field_no_revision_cache-456488-6.patch14.56 KByched
#5 field_no_revision_cache-456488-5.patch14.48 KByched
#3 field_no_revision_cache-456488-3.patch14.16 KByched
#2 field_no_revision_cache-456488-2.patch18.48 KByched

Comments

bjaspan’s picture

bjaspan commented

I have no objection to this. The whole reason we moved field revision storage to separate tables is to make the more important activity of using the current revision more efficient.

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new field_no_revision_cache-456488-2.patch18.48 KB

Patch attached.
- Adds a few checks to the existing testFieldAttachCache(),
- abstracts random values generation, used all over FieldAttachTestCase tests, to a helper function.
- removes an unneeded check for isset($info['cacheable']) in _field_attach_extract_ids() - _field_info_collate_types() ensures there's a default value.

yched’s picture

yched commented
StatusFileSize
new field_no_revision_cache-456488-3.patch14.16 KB

Oops, not sure what happened, previous patch had borched indentation.

bjaspan’s picture

bjaspan commented
Status: Needs review » Reviewed & tested by the community

Very nice.

yched’s picture

yched commented
StatusFileSize
new field_no_revision_cache-456488-5.patch14.48 KB

_field_attach_load() : We now check for $cacheable outside the "foreach(object) {fetch from cache}" loop, but I forgot to remove the previous test inside the loop.

yched’s picture

yched commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new field_no_revision_cache-456488-6.patch14.56 KB

Rerolled after #362024: Field Attach: Make hook_field_load() be 'multiple' like field_attach_load(). CNR for the bot.

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

yched commented
Status: Needs work » Needs review
StatusFileSize
new field_no_revision_cache-456488-8.patch14.56 KB

Small foobar while manually rerolling the testFieldAttachCache test. Should be exceptions-free now.

yched’s picture

yched commented
StatusFileSize
new field_no_revision_cache-456488-8.patch14.56 KB

Doesn't seem to apply cleanly on webchick's setup, reroll
+ fixes one more missing t() in test asserts.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Ok, cool. yched walked me through this one a bit as well.

Aside from some minor neatness tweaking here and there, the patch basically does two things:

- Changes caching to be based on the entity ID rather than its revision ID, so that we greatly reduce the bloat of what we're caching and get all the benefits outlined in yched's initial post. This is also consistent with the latest iteration of the node_load cache patch, so we'd be consistent with that if it gets committed.

- Creates a new _generateTestFieldValues() function which encapsulates the typical:

-      for ($delta = 0; $delta <= $this->field['cardinality']; $delta++) {
-        $values[$revision_id][$delta]['value'] = mt_rand(1, 127);
-      }

logic found in just about every test. This makes a nice clean-up which makes the test code easier to read.

Maybe I'm getting tired (or maybe this is a fantastic patch! ;)), but I couldn't really find much to complain about here. It's just re-ordering the logic a bit and changing the affected sections of the code, and has both field API maintainers' thumbs up on it.

I fixed one minor indentation thing and then committed to HEAD. Thanks!

yched’s picture

yched commented
Status: Fixed » Needs review
StatusFileSize
new field_no_revision_cache-456488-10.patch4.09 KB

Er, oops. My reroll in #8 reverted some cleanups in testFieldAttachCache() that were added in #362024: Field Attach: Make hook_field_load() be 'multiple' like field_attach_load().

The idea is that when doing a series of field_attach_save(), field_attach_load(), field_attach_update(), we should reset the $entity each time, so that $entity->field_foo entries set in a previous step do not pollute the next ones. Those changes were in the 'field_attach_load() multiple' patch, not out of pure sadism towards kittens, but to fix actual test hiccups with the patch.

CNR for the bot, but should be RTBC if the bot comes out green.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Tests OK.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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