Problem/Motivation

As a follow-up #3562989: Implements RevisionLogInterface for Instance entity. We are calculating the hash everytime we need (for display in LogsPanel or to check data changes):

class Instance extends ContentEntityBase implements InstanceInterface {

  public function getPublishedHash(): ?int {
    $published_data = $this->getBuildablePlugin()->getSources();

    return $published_data ? self::getUniqId($published_data) : NULL;
}

The logic is simple but it seems costly.

Proposed resolution

Because data is more often retrieved than saved, can we calculate the hash when we save the data instead ?

It will increase logic complexity (we need to save in both the live instance and in the permanent "published" storage) but it may increase performance.

How can we evaluate this performance gain, in order to decide if it is worth the hassle?

Issue fork display_builder-3595491

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

pdureau created an issue. See original summary.

pdureau opened merge request !289

pdureau’s picture

pdureau commented
Status: Active » Needs work

Some pipeline fails, but there are OK on my local environment. Weird.

pdureau’s picture

pdureau commented
Assigned: pdureau » Unassigned
Status: Needs work » Postponed

Some pipeline fails, but there are OK on my local environment. Weird.

It may be because my local environment follows ui_patterns:2.0.x when CI needs ui_patterns:2.0.15 because if i switch locally to ui_patterns:2.0.15, I have the same fails.

Let's wait ui_patterns:2.0.16

pdureau’s picture

pdureau commented
Assigned: Unassigned » mogtofu33
Status: Postponed » Needs review

Hi Jean,

I have rebased this ticket which is now ready to be discussed.

Because data is more often retrieved than saved, this is proposing to calculate the hash when we save the data instead if every time we retrieve data.

It will increase logic complexity (we need to save in both the live instance and in the permanent "published" storage) but it may increase performance. How can we evaluate this performance gain, in order to decide if it is worth the hassle?

mogtofu33 made their first commit to this issue’s fork.

pdureau’s picture

pdureau commented

Careful, the last rebase has restored stuff already removed from 1.0.x and not added in this work, like Instance::isPublishable()

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs review » Needs work

Pushed small bug fixes and logs improvements.

Base on claude estimate performance gain is not worth it and we could have a gain somewhere else:

I measured this to answer the open question. Numbers from the demo site, PHP 8.5, single request, 1000 iterations per case.

Cost of what we would remove

Operation Cost
Instance::getUniqId() on the biggest real display (node.landing_page.default, 12 KB serialized) 11 µs
Same tree x10 (120 KB) 109 µs
Same tree x100 (1.2 MB, not a realistic display) 1.13 ms
Config entity load, static cache hit 17 µs
Config entity load, cache reset first 185 µs

Real sources trees on this site range from 2 KB to 12 KB. Hashing one costs 11 µs.

How often it actually runs

There are three call sites, all of them islands: SaveStatus::restingStatus(), StateButtons and LogsPanel. Nothing in the list builders, and nothing in front-end rendering: displaying a node never computes a published hash. So the total is about three hashes per builder render or htmx partial, on an admin screen that already costs hundreds of milliseconds.

Expected gain

  • Entity view and Views displays: around 33 µs per builder render. That is noise.
  • Page layouts: also around 33 µs, because PageLayout::getStoredHash() still calls getSources(), which deliberately drops the memoized entity so the config entity is reloaded at 185 µs per call. The reload is the real cost in that path, and storing the hash does not remove it.

So on these numbers, no, the gain is not measurable. The hash was never the expensive part.

A cheaper option for the same problem

Memoizing the computed hash on the buildable plugin for the duration of the request collapses the three calls into one. No new config key, no schema change in three modules, and no risk of a stored hash going stale when sources are written outside saveSources() (a hand edited YAML, drush cset, another module touching the third party settings). That captures whatever gain exists here at a fraction of the complexity this issue was already worried about.

If we want a real win in this area, the target is PageLayout::getSources() discarding its memoized entity on every call. That single reload costs 17 times more than the hash it feeds.

Notes on the MR itself

The design is sound and backward compatible: getStoredHash() returning NULL falls back to computing, so existing config keeps working without an update hook. Three issues found while reviewing it, all fixed locally:

  • EntityView::getStoredHash() read $this->entity directly. That property is lazy and is still NULL when the plugin is built from display_id configuration, so the stored hash was never used and the code silently fell back to recomputing. Now uses $this->getDisplay()?->getThirdPartySetting(...).
  • ViewDisplay::getStoredHash() had the same problem with $this->extender, which is NULL until getExtender() runs, so it emitted "Attempt to read property on null" and returned NULL. Now uses $this->getExtender()?->options[...] ?? NULL.
  • LogsPanel moved from computing the published hash once to calling $step->isPublishedPresent() per row, and again per step in printSaveAlert(). That is a regression in the exact panel this issue cites, and on page layouts it meant one config entity reload per row. The hash is resolved once in build() again and passed down.

Also worth deciding before this lands: Instance::getPublishedHash() now returns int, while PublishableInterface still declares ?int and documents "NULL if the display has never been published". The docblock needs updating either way.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33

Memoizing the computed hash on the buildable plugin for the duration of the request collapses the three calls into one. No new config key, no schema change in three modules, and no risk of a stored hash going stale when sources are written outside saveSources() (a hand edited YAML, drush cset, another module touching the third party settings). That captures whatever gain exists here at a fraction of the complexity this issue was already worried about.

No schema change ? No new method in Interface ? Sounds great. So memoization with something like new DisplayBuildablePluginBase::$hash? We can get rid of the current proposal, and start a new one going this way. Can Claude propose something?

If we want a real win in this area, the target is PageLayout::getSources() discarding its memoized entity on every call. That single reload costs 17 times more than the hash it feeds.

Yes, but it was done in purpose in #3616225: Buildable plugins bypass their own dependency injection, doesn't it?

  public function getSources(): array {
    // Drop the memoized entity so ::getEntity() reloads it: the Drupal cache
    // is very strong on this data.
    $this->entity = NULL;

    return $this->getEntity()->getSources();
  }
Also worth deciding before this lands: Instance::getPublishedHash() now returns int, while PublishableInterface still declares ?int and documents "NULL if the display has never been published

Yes, we need to decide. DisplayBuildableInterface::getSources(): array; is not nullable, so will return an empty array if empty.

The hash of an empty array (\crc32((string) \serialize([])))) is 2723407904, so not NULL (by the way, the hash of a NULL value is also an int: 265346081)

So, Instance::getPublishedHash() is nullable only because the very own implementation is nullable:

  public function getPublishedHash(): ?int {
    $published_data = $this->getBuildablePlugin()->getSources();
    return $published_data ? self::getUniqId($published_data) : NULL;
  }

This may be better

  public function getPublishedHash(): int {
    $published_data = $this->getBuildablePlugin()->getSources();
    return self::getUniqId($published_data):
  }

mogtofu33 opened merge request !353

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review

I profiled this before implementing. The hash is not where the cost is.

One getPublishedHash() costs 36 µs: 10.7 µs of crc32(serialize()) and 22 µs of PluginItem::getInstance() calling createInstance(), which rebuilds the buildable plugin and reloads the config entity it wraps. Storing the hash does not remove that 22 µs, since reading a stored hash still needs getDisplay() or getExtender().

Approach First call Later calls Three calls (one render)
Current 1.0.x 36 µs 36 µs 108 µs
Stored hash ~22 µs ~22 µs ~66 µs
Memoization 36 µs ~0 µs 36 µs

Doing both would save a further 14 µs once per request, in exchange for a config key in three schemas and a hash that can go stale. Not worth it, so I went with memoization only.

Pushed to 3595491-hash-memoizing

  • PluginItem::getInstance() memoizes the plugin for the lifetime of the field item, reset in a setValue() override. On the field item rather than the entity, because ContentEntityBase::__sleep() drops field objects and keeps the plugin out of any serialized entity.
  • Instance::getPublishedHash() memoizes the resolved hash, invalidated in ::publish() and ::revert().
  • LogsPanel::printSaveAlert() reuses the hash the caller resolved. Each step is a separate revision object, so that loop was one plugin construction per step.

Repeat getPublishedHash() goes from 27.5 µs to 0.03 µs, isPublishedPresent() from 29.9 µs to 0.65 µs. Three files, 49 lines, no schema change, no update hook. Kernel 349 tests and unit 42 green, including InstancePublishingTest which walks publish, restore and revert. phpcs, PHPStan and PHPMD clean.

Follow-up

  • Proposing we close the stored-hash MR as superseded, unless we want it for correctness rather than speed: a hash taken from the in-memory tree at save time sidesteps the normalizeRootLevel() drift between the two storages. That is a different argument and should be a different issue.
  • PageLayout::getSources() sets $this->entity = NULL on every call, so page layouts reload the config entity each time and get no benefit from the memo. It is now the biggest cost left in this path and the only justification in the code is "the Drupal cache is very strong on this data". Worth its own issue.
pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33

Pushed to 3595491-hash-memoizing

I don't see anything: https://git.drupalcode.org/project/display_builder/-/merge_requests/353/...

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau

Oups, pushed.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs review » Reviewed & tested by the community

Thanks

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • mogtofu33 committed 19890526 on 1.0.x 
    task: #3595491 Hash memoizing

By: pdureau
By: mogtofu33

Status: Fixed » Closed (fixed)

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