Problem/Motivation

A pre-emptive cache set was added to ThemeRegistry in 2013, CacheCollector has a lot of protection against race conditions since then.

Steps to reproduce

Proposed resolution

Bring ThemeRegistry closer into line with CacheCollector and drop the explicit cache set in the constructor.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3587707-nr-bot_sc6op_nd.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3587707

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

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
Issue tags: +Performance

Yeah this looks completely unnecessary, at least now if not necessarily in 2013 when it was added. Saves a lock acquire + release + cache set + cache get on cold cache requests.

catch opened merge request !15616

catch’s picture

catch
he/him
Primary language English
commented

The complete override of the ::updateCache() method also looks unnecessary.

As well as that we can probably get rid of the checks for whether modules are loaded now that OOP hooks is in, but that should probably be its own issue since it's not strictly related here, and speaking to nicxvan it might depend on procedural hooks being unsupported which is a bit further ahead.

catch’s picture

catch
he/him
Primary language English
commented

Was able to remove most of the updateCache override here and simplify the constructor slightly too.

catch’s picture

catch
he/him
Primary language English
commented
Issue summary: View changes
nicxvan’s picture

nicxvan commented

I think that has to wait for that last bit and since all of core has been converted we would need contrib to confirm real world scenarios.

This issue looks like a nice clean up, did you find it looking through the queries in the performance tests?

catch’s picture

catch
he/him
Primary language English
commented

I did dump($this->getCacheOperations()) on the node cold cache performance test - had a quick look for anything appearing more than once or similar things that could possibly be multiple loaded. The runtime cache entry was being got least three times and set twice.

#3587565: Static cache entities that aren't loaded and #3587601: Avoid loading date formats in element info from yesterday also came out of that.

As with a lot of things, it gets easier to spot issues like this the more previous issues have been fixed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3587707-nr-bot_sc6op_nd.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Needs review

Rebased.