Bumping to critical since this prevents working on the test locally at all, which is how I found it.
Good plan to use git bisect, I didn't really think this would have been broken by a commit except maybe a phpunit change.
I can confirm that 2999d4574c4, the commit immediately before 5d387a, passes locally for me.
If I revert the system libraries change against 5d387a, this also passes.
However, if I revert the same change against 11.x, that does not help. (!!!!)
That CSS (unless my grep is completely failing) is only used in Claro, which is not used by this test. However, it would be loaded in all functional tests even if it's unused. That combined with a straight revert against 11.x not working makes me wonder if it's not necessarily the specific CSS rules being there or not that was the problem but the file itself or some other bizarre side-effect of that change. And that other changes since then mean that adding back the CSS/file doesn't make the test work again.
Adding a revert MR not because reverting works for me yet, but in case someone is able to confirm that the revert looks correct but also doesn't fix the issue.
📌 Allow for / implement simplified content workflow with workspaces Active will put all content created on a moderated node form into a workspace, and then when the node revision is published, transition that content too, including menu links, media items, path aliases etc. I haven't reviewed the MR here, but it will only be temporary until that work is finished (which I think is one or two issues away from completion, a lot landed already).
Added the dynamic_page_cache check and unit test coverage for it.
BigPipeTest failures I cannot debug locally due to 🐛 Local test failure - BigPipeTest Active and is a bit of a mystery.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
We should make it so this behaviour only happens with dynamic_page_cache module installed probably.
The reason for this is the additional request required before a page/external cache hit:
Request 1 - render caches populated, uncacheable response due to big pipe.
Request 2 - render caches warm, big pipe doesn't run, page cache is set but also MISS
Request 3 - page cache is warm.
With dynamic_page_cache request 2 is not quite as fast as an internal page cache or CDN/varnish hit, but it's still very fast. Without it a lot less is cached, and there's also more chance of big pipe running on the page (for something that's not independently cached outside the dynamic page cache).
This will likely mean undoing some of the test changes already made, like the unit test...
Fixed a couple of tests, BigPipeTest fails for me on HEAD locally so I opened 🐛 Local test failure - BigPipeTest Active - a bit hard to debug when it doesn't get to the CI fail.
Didn't touch the performance test failures yet, the differences are all expected I think.
@oily #110 explains why unconditionally adding the block title as a heading could cause problems, and suggests only conditionally adding it when label display is off, see the start of that comment.
@oily the proposed solution here is in #110. The suggestion you applied doesn't match that. If you'd like to try implementing #110 instead that could help get this issue done.
This happened in 📌 Aggregate cache operations per bin to allow for more specific asserts Active
This looks great, can't see anything else to complain about. We can convert the one constructor to property promotion separately.
Easy to forget this happens on every cold cache request regardless of whether ckeditor appears on the page or not - because it's called when building library definitions which is global.
Hmm I don't think my branch was out of date, but maybe the tab with the diff in it was or something? Either way that's great and committed/pushed to 11.x and 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
This doesn't apply due to 📌 Optimize EntityFieldManager::buildBundleFielDefinitions() Active which knocks another 20 queries + cache gets + cache sets each off the same test.
On the plus side, once this lands we're looking at about 200 less queries / cache sets / cache gets / cache tag lookups combined reduction in the umami front page cold cache test compared to 11.2.x, which is really a huge reduction in less than six months.
Fair enough about the argument type hint. Let's just swap the phpstan baseline entry for @return docs.
https://www.drupal.org/project/drupal/releases/11.2.7 → is packaging now.
Went ahead and committed 11.2.x from needs review since it is only a partial revert, so that the merge pipeline can start running. Retrospective reviews would be welcome given we want to get the paperbag release out today, and it would be good not to be making a third release tomorrow after doing something silly.
The MR was backported to 11.2 because it fixed an earlier regression in 11.2, otherwise I wouldn't have backported it at all. Possibly we should have lived with the regression until 11.3.0 but I definitely didn't think that anyone would be implementing __get() in a custom entity storage class already, or that the change would break it.
I think it's safer to go forwards and commit the fix here than try to undo the change in core. We could update core to try to undo the break, but would prefer to avoid a paper bag release if we can.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
I think the new docs and the 11.3.0-alpha1 release notes snippet is good. For Drupal 12 the same text seems fine, we can just change 'is deprecated' to 'is not supported'.
Committed/pushed to 11.x and cherry-picked to 11.3.x and 10.6.x, thanks!
📌 User #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe Needs review landed, this is worth a try now I think.
I've added a summary of #38 to the release notes for https://www.drupal.org/project/drupal/releases/11.2.6 → and https://www.drupal.org/project/drupal/releases/10.5.5 →
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Note that there have been several improvements to block rendering in block module in 11.1-11.3, most recent is 📌 User #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe Needs review (currently RTBC), all of these make equal sense for when blocks are placeholdered in Canvas too.
Every time we have a new major release, we have to retarget thousands of MRs (and issues) to the new major branch, e.g. 10.x to 11.x, or 11.x to 12.x.
With a 'main' branch, all issues and MRs, except for backports, will target 'main' in perpetuity.
The '11.x' branch itself was a move towards this - before we opened that, we used to have to retarget issues and MRs everything six months when a new minor branch opened, we only started using 11.x because some d.o things blocked using main, which they now don't.
#7/#8 mystery is solved by 📌 Stabilize OpenTelemetryAuthenticatedPerformanceTest Active .
With that there's no cache/database change here at all, but we do save loading about 50kb of JavaScript on an authenticated warm cache request - because Big Pipe is taken out of the equation.
The other benefit is that pages that don't require big pipe are varnish/CDN-cacheable, which enables 📌 Allow big pipe to run for session-less users Active .
Going to be optimistic and bump to major - this could make a big difference to percieved performance while caches are warming up.
#5 isn't a problem at all if we do 📌 User #placeholder_strategy_denylist for CacheOptionalInterface blocks to prevent them being rendered by BigPipe Needs review .
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Did some before/after profiling and got about 9ms with 11.x, and about 4.5ms with the MR - for me the function shows up in all xhprof runs, wonder if there's some kind of environment/config difference between berdir's and my xhprof setups.
But either way that confirms it's a good performance improvement as well as simplification.
OK this is now committed/pushed/cherry-picked to 11.x, 11.3.x, 11.2.x, 10.6.x, and 10.5.x.
I think it might be worth a follow-up to split the individual vs collection logic into two methods if we can, then we could decide if we want to deprecate the collection logic. Anything that needs multiple latest revisions could individually request the latest revision for each one and skip multiple loading.
Glad we were able to finally find a good solution that works for both kinds of large datasets.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
xhprof shows everything, you get a very long list but it does help to rule out profiling the completely wrong thing.
Instead of clearing cache, I prefer to set the page/dynamic page cache/render bins to null
That's more organised than me.
Without actually testing it, I think that using the front page or node page of Umami (both of which have views on), and truncating the cache_dynamic_page_cache and cache_render tables prior to loading the page should mean the code runs.
In xhprof if you view the full list of functions, need to make sure initDisplay() is actually in the list somewhere, otherwise it's possible the page didn't execute it at all and have to tweak the steps to reproduce.
What I usually do is screenshot the xhprof detail for the function (at the highest level of the stack that makes sense, which is probably initDisplay()) before and after. It's very imperfect but never found a good way to host/display xhprof runs in a persistent way online.
Updated the comment.
I wondered about permissions before looking at how this is generated, but we would need 100% generic revision view permissions across entity types, which... maybe eventually.
On trying to share the same font file as Navigation, looked into this with @mherchel in slack https://drupal.slack.com/archives/C4M1EV8G5/p1762255343867119
Not 100% confirmed but it looks very likely that browsers (at least chrome) can recognise when a font-family declaration is for the same font, and avoid re-downloading the font file. However, that appears to be broken by font-style or at least when one declaration has font-style and one doesn't.
font-style isn't required with a variable font though, so if mercury shipped with a variable font, then browsers should be able to de-duplicate that font for users of the Navigation bar. Small saving because it's admin-facing, but a nice side effect of switching to a variable font that hopefully should avoid further fine tuning.
While researching that I was reminded of src: local(Inter) which will look for a system font before it tries to download the shipped one. Only helps a subset of users but not bad for a single line of CSS. Need to open a core issue for that one too.
JSON:API tests are quite awkward to debug when anything changes, but are now green.
Per #109 I've committed the 10.6.x MR and cherry-picked it to 10.5.x so that 10.x is on a level playing field with 11.x
We can continue in 📌 Latest revision subquery optimisation Active which is a significant improvement again over this issue. And also 📌 Don't check if we're on the latest revision before adding the working copy link Active which is down to one or two test fails.
@yakoub you've had several responses and you've just brushed them off with non-sequiturs.
I replied to you in #3548313-71: Updating to 10.5.3 causes gateway timeouts on revisioned content → with an example of where we're trying to completely factor out use of this query in content_moderation module (no lookups of latest revision at all in the common code paths, not even for individual entities), and also opened 📌 Don't check if we're on the latest revision before adding the working copy link Active which would completely remove some calls from JSON:API. Your reply to that was 'I have to disagree' although it was not clear with what you were disagreeing.
More to the point, adding a separate API to get only the latest revision of an entity ID and potentially drop the case of allowing queries against multiple entities and deprecating the current API is something we could only do in a minor release, and we want to fix the regression for 10.5.3 sites asap, without re-introducing the slow query we had prior to 10.5.3, ideally this week.
I started working on a revert but then thought about it more:
Original query - slow with lots of revisions per entity but did not bring sites down.
🐛 Entity queries querying the latest revision very slow with lots of revisions Needs work fixed original query for lots of revisions per entity, but very bad with lots of entities to the point it could bring sites down.
Status quo following the commit here: Still possibly slow for lots of revisions per entity, although not necessarily worse and possibly better than the original query, fixes the regression we introduced in 🐛 Entity queries querying the latest revision very slow with lots of revisions Needs work .
I think the original state and the current state are better than the regression, since we don't have indication that the current state is worse than the original slow query, I think we should stick with where we are until we work out something better in 📌 Latest revision subquery optimisation Active .
Additionally 📌 Don't check if we're on the latest revision before adding the working copy link Active will (dramatically) reduce the frequency that this runs on sites with JSON:API enabled, which will be a small improvement once everything is fixed for all possible versions, but could be a bigger improvement in the interim.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Given the overall timings this looks like something we should try to get into 12.0.0
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
I still have the same question as #33, which I've also asked on the MR. ::optimizeGroup() should only ever run for files with preprocess: true, and there isn't an explanation here on how that ends up running for files with preprocess:false.
The unit test shows that you can get to this code path in a unit test, but it does not show how you can get to it in core + a library definition.
Are there steps to reproduce that happen only with a module like google tag manager and core? I think this probably needs a functional or kernel test to demonstrate the issue.
If it can't be reproduced with core + a module with a suitable library definition, then it could be a contrib module interfering, maybe advagg or similar?
Asked for a comment on the test change, but @longwave pointed out that while it's the odd one out now, it won't be when we remove the bc layer, so no comment is needed indeed.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Discussed this briefly with @alexpott and DrupalCon Vienna and he had a similar but potentially much easier idea than what's currently in the issue summary.
Instead of using a build test to generate a database dump on an older Drupal version, we could use a build test to install Umami, and then update that installed site.
We can probably install modules and create content on the installed site prior to updating it if we need extra data to test a specific update too.
@smustgrave it should theoretically be possible to check a branch run on PHP 8.5 against HEAD vs one from the MR here and compare the two, to avoid having to set up PHP 8.5 locally, this might have saved some time although I'm not sure how discernable the PHP 8.5 test failures are with the amount of deprecations messages we have.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
https://git.drupalcode.org/issue/drupal-3554646/-/jobs/7142642 is 100 green runs of this test.
https://git.drupalcode.org/issue/drupal-3554646/-/jobs/7143359 is a second run I'm hoping will be green.
Answered the review comments on the MR.
Had a look at Mercury's font situation, it also uses Inter.
Mercury isn't using a variable font (but probably could/should), however it is splitting the glyphs - they're added back via separate CSS declarations using unicode-range (https://developer.mozilla.org/en-US/docs/Web/CSS/@font-face/unicode-range) which answers #16. This is done for cyrillic, greek, and vietnamese.
So I think for Navigation, splitting the variable font by glyphs then adding them back with unicode-range might end up being the most efficient.
@larowlan that's about as far as I got thinking about what it would look like, but yes either in theme.info.yml or a new theme.default_blocks.yml. It would allow themes to have a minimal working configuration even when nothing else is installed to place blocks in regions.
A variable font combines regular/italic/bold into a single file, which is usually smaller than providing them separately (if they're actually used, but it looks like at least two styles are). Also potentially cuts down on latency.
Navigation is already using the variable version of Inter.
Drupal core does both bundling and minification on the fly, but it can only do so for assets added via the library system which these are not.
Figured it out.
The cache warming requests for asset aggregates/image derivatives on the node page weren't exactly the same as the one we're recording cold cache backend performance for - because the test logs in a user first and that warms some different caches and assets prior to warming the node page. So there was at least one aggregate that was unique to the performance recording request, which causes an extra Drupal request and bootstrap etc.
The solution is to visit node/1 with an empty cache after logging in, then the same asset libraries are requested and the aggregates created.
So this adds an extra uuid lookup because we still need to do ✨ Add static cache for loadEntityByUuid function to store uuid-id pairs in memory Active but it does clearly remove the problematic query from the other issue, which would explain why it was bringing sites down that weren't explicitly requesting the working copy.
@pwolanin are you able to post the slow database query that goes along with the 60s request from #85?
Committed/pushed to 11.x and cherry-picked to 11.2.x, thanks!
Will need a backport MR for 10.6.x
@dww this issue was opened because trying to get the right email address for people was turning into a huge amount of work. See the postponed related issues.
I have two no-reply addresses associated with my d.o account as well as my personal one.
I don't think @username is a gitlabism, it's a common approach across social media and various ticketing systems at this point.
The main cause of the latestRevision query in core, at least it running frequently, is the 'load_latest_revision' route param upcaster, which is added to node routes by content_moderation module as @alexpott points out.
📌 Allow for / implement simplified content workflow with workspaces Active is one or two steps away from making that a non-issue (by making content_moderation depend on workspaces, which tracks draft revisions rather than only dealing with the 'latest', and therefore can remove that upcaster from those routes), but there will still be contrib uses around to get rid of after that and the handful of individual calls to it in core.
📌 Use simple query mode for entity queries with limit 1 and no offset Needs review may be relevant here since it's trying to simplify single-entity queries.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
We might want a follow-up to remove the rule again once we require PHPUnit 12/13 in Drupal 12, when presumably the annotations will be long forgotten.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!