Updated: Comment #0
Problem/Motivation
Entity rendering got a lot slower in Drupal 8 (citation/numbers needed). That's why we worked very hard on #1605290: Enable entity render caching with cache tag support. That issue solved the problem for most entity types… but not for the most commonly used entity type of all: nodes.
Render caching for nodes is blocked on:
#914382: Contextual links incompatible with render cache(yellow because work on a D7 backport is being done, it is fixed in D8- #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user
- #1605290: Enable entity render caching with cache tag support
- #2099133: Comment form on same page as comments forces node render caching to be per user
- #2090783: Run comment op links (delete, edit, reply, approve + contrib) through #post_render_cache to prevent render caching granularity being per-user
- #2151439: Run node op links ("Read more", "Add child page", "Printer-friendly version", "X views" + contrib) through #post_render_cache to prevent render caching granularity being per-user
- #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links
- #2151457: Comment pager breaks the render cache
Many of these only became fixable after #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache landed a few weeks ago. We're almost at the point where can finally enable node render caching :) That will happen here!
Proposed resolution
Enable node render caching once all blockers have been fixed.
Remaining tasks
Fix all blockers.
User interface changes
None.
API changes
None.
Comment | File | Size | Author |
---|---|---|---|
#80 | drupal_render_no_comment.png | 89.7 KB | Berdir |
#80 | full_no_comment.png | 38.43 KB | Berdir |
#80 | drupal_render.png | 85.71 KB | Berdir |
#80 | full.png | 39.99 KB | Berdir |
#78 | enable_node_render_cache-2151459-78.patch | 38.38 KB | jessebeach |
Comments
Comment #1
xjmDiscussed with @effulgentsia, @Dries, @webchick and others. It's important to expose contrib authors to this concept early and this is going to have an important impact on performance, so we are categorizing this as a beta blocker.
Comment #2
catchIf this is a beta blocker, I'd probably add #2099131: Use #pre_render pattern for entity render caching as well -that's going to affect when certain hooks run and possibly what's available to them just as much as render caching at all.
Comment #3
amateescu CreditAttribution: amateescu commented#drupal8wins! :)
If we have any test failures when all the issues from the OP are committed, we probably need to add some of the manual cache clears from the interdiff in #1605290-135: Enable entity render caching with cache tag support.
Comment #4
Wim LeersI can't believe this patch comes down to *so* few lines! I started looking at the #1605290: Enable entity render caching with cache tag support patch that got committed, looking for things you might've forgotten. But if you did, I don't see it!
And AFAICT even zero tests need to be written, the
\Drupal\system\Tests\Entity\EntityRenderTest
that was added in #1605290 seems sufficient.Wow :) A 2-line patch that will hopefully unlock quite the performance boost!
Comment #5
Wim Leers#2151457: Comment pager breaks the render cache landed 3 weeks ago, #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links is good to go but needs a review to get to RTBC. #1979468 is the last blocker for this patch. Please review it!
Comment #6
BerdirLet's try it then.
Comment #7
Wim LeersOMG YAY! #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links has landed, which unblocks this!
amateescu's patch is perfect, and still applies.
However, I found one
EntityViewBuilder::resetCache()
call that shouldn't be happening: rendered nodes that contain rendered comments should have cache tags to reflect that they contain rendered comments (and that's in fact the case), so we shouldn't be doing that separately ourselves.In other words, I don't see why the comment field type needs to do this whenever its settings are changed:
If the comment field type needs it, then every field should need it. And not a single other field type has it.
(I discovered this in #2151457: Comment pager breaks the render cache but didn't want to delay that patch any longer.)
Comment #10
catchI don't think we should tag nodes with all their comments. Forum sites for example can have several thousand comments on a single issue, which won't scale - the comment references the node, not vice-versa.
If we had a unique comment thread ID we could tag nodes with that then clear it when a comment in that thread is updated/inserted - might work.
Comment #11
Wim Leers#10: Wow, I did not see that coming :) Changing the cache tags on nodes is completely out of scope for this issue IMO. Let's get this in first, and only then worry about finalizing/fixing/optimizing our use of cache tags.
The patch in #7 only removes something that was unnecessary, in the current state of cache tags.
To reply to your actual points, but please let's not derail this issue with this discussion:
Comment #12
catchReplying under-the-line ;)
Whether threading is enabled or not, adding new comments/updating/deleting can change what will be rendered on other pages as well - especially when comment threading is enabled but not only. So if we just used a tag for this, something like the comment field name + entity ID would be the closest (i.e. the specific set of comments that the one being added/updated/deleted appears in).
Comment #13
Berdir7: enable_node_render_cache-2151459-6.patch queued for re-testing.
Comment #15
amateescu CreditAttribution: amateescu commentedAs I suspected, we need to bring back a few parts from the interdiff in #1605290-135: Enable entity render caching with cache tag support. I also moved the 'in_preview' special casing to the NodeViewBuilder because this doesn't apply to all entity types.
Drupal\comment\Tests\CommentAnonymousTest
will be fixed by #2099105: Clean-up render cache when permission changesNote sure what's the best way to fix the failures in
Drupal\statistics\Tests\StatisticsAdminTest
as I'm not familiar with how the statistics ajax thing works, maybe we just need to clear the render cache in the test?Comment #16
catchDidn't look into the specific failures, but it case it helps: the AJAX handles collection, but statistics are displayed by PHP. So cached rendered nodes could have stale statistics counts. I think this is fine and we could manually clear the render cache for tests. If someone decides it's not fine at a later point, then it's another use case for #post_render_cache.
Comment #17
BerdirNot sure why the first one can be removed and the other not?
Isn't the point of this that there can be comment setting changes that affect how comments are rendered (e.g. changing the pager id) that can't be catched through cache tags?
And yes, maybe it should be done on all changes.
If you look at field.module, it currently has tons of calls to clear the field (soon entity) caches, so maybe that should be extended to clear the render cache as well? And possibly move to the entity system with #597236: Add entity caching to core
Not related to this but wondering if there is a reason the API for the render cache resetCache() and the storage is different (just ID's vs. complete objects)?
Comment #18
amateescu CreditAttribution: amateescu commentedOops, I forgot that I need post things under-the-line :D
Re #16:
I'll let @Wim answer that as he's probably more familiar with the statistics stuff. I'm inclined to say that we do want to implement #post_render_cache for view counts.
Re #17:
Because the 'parent' entity (e.g. node) render cache is also tagged with the comment IDs, so it gets cleared by that virtue.
We need the full objects here because we have to get the bundle of the entity. See #1712456-19: How to leverage cache tags in Views :)
Comment #20
amateescu CreditAttribution: amateescu commented15: enable_node_render_cache-2151459-15.patch queued for re-testing.
Comment #22
amateescu CreditAttribution: amateescu commentedRerolled.
Comment #24
Wim LeersThis issue should wait until #1979468: ".active" from linkGenerator(), l() and theme_links() forces an upper limit of per-page caching for all content containing links lands again (it was reverted).
Comment #25
jessebeach CreditAttribution: jessebeach commented[1979468-258] was recently committed. This issue is unblocked now.
Comment #26
jessebeach CreditAttribution: jessebeach commented22: enable_node_render_cache-2151459-22.patch queued for re-testing.
Comment #28
jessebeach CreditAttribution: jessebeach commentedRerolled.
The following failed to apply in NodeType.php
Comment #31
dsnopek28: enable_node_render_cache-2151459-28.patch queued for re-testing.
Comment #33
Wim LeersOn it.
The failures are legitimate. The statistics-related failure I should definitely be able to figure out. The search ones hopefully also.
Comment #34
jessebeach CreditAttribution: jessebeach commented28: enable_node_render_cache-2151459-28.patch queued for re-testing.
Comment #35
jessebeach CreditAttribution: jessebeach commentedThe test fails noted in #28 for
SearchMultilingualEntityTest
just passed on my local setup, so I've sent the patch for retesting. Maybe something changed in the matrix and these now pass. I mean, it has been 6 days since those tests were first run.Comment #37
jessebeach CreditAttribution: jessebeach commentedThe Statistics module test failures are indicative of a gap in the
drupal_render()
testing. I've added what I hope is an additional test that should fail until we've addressed the issue plaguing the Statistics' tests -- namely, that jumping into the render tree midway for an entity (such as when links are rendered from a#prerender
function) or when a property of an entity is rendered separately from the top-level node (such as content.links in the node template) causes the post render cache placeholder to be replaced with content and then cached. We want instead for the placeholder to be cached and replaced on render outside of a cache hit.I simulated jumping into the render tree at the child level with this code:
Comment #38
Wim LeersI made the modified & added tests consistent with the others. I've figured out a work-around. The tests jessebeach added now pass. Statistics are now working fine too, and those tests now pass also :)
Also, I failed to update the comment module's node links in #2151439: Run node op links ("Read more", "Add child page", "Printer-friendly version", "X views" + contrib) through #post_render_cache to prevent render caching granularity being per-user. Fixed that in this reroll also (see the separate interdiff for that).
Comment #41
jessebeach CreditAttribution: jessebeach commentedJust a small hunk rejection in
comment.module
over a conflictinguse
statement addition.Comment #43
jessebeach CreditAttribution: jessebeach commentedThis should clear up the exceptions.
Comment #44
jessebeach CreditAttribution: jessebeach commentedComment #46
BerdirInteresting, looks like that code path did never run before or works somehow different now.
comment_preview() incorrectly messed with the field on the commented entity, first it got the whole field (field item list), then it set the status, which due to internal defaults/magic ends up on $field->status, and then sets the whole field item list the same way, which also ends up on ->status, so you have $entity->$field->value = $field :)
Maybe the Entity Field API should better handle that and support this but I think the changed code makes more sense anyway, but is unfortunately a bit problematic, apparently:
If I understand it correctly, the new hook now runs after we set the original value back, because it now runs when the render array is rendered. That could be rather unexpected. Maybe we should clone the entity instead, then we don't have to worry about setting back the original value?
Comment #48
jessebeach CreditAttribution: jessebeach commentedThis is EXACTLY the behavior I encountered in #2099131-23: Use #pre_render pattern for entity render caching!! I thought it was something I did or neglected to do in that patch, but now I think it goes deeper.
Comment #49
larowlanPlease forgive my incorrect use of the ->get/->set, the comment makes it clear what it is supposed to do, and the interdiff at #46 works for me, I wasn't aware that get/set worked differently. I am now. Thanks.
Comment #50
Wim LeersFixed the failure in
Drupal\node\Tests\NodePageCacheTest
.Drupal\search\Tests\SearchMultilingualEntityTest
passes locally… even though I didn't touch it at all. So here's hoping it will pass on testbot too.Comment #52
BerdirI can reproduce, but only when I execute the tests with run-tests.sh. Not sure what's going on there, but the translations are somehow switched, the 0/1 keys there.
Comment #53
Wim LeersFollowing berdir's advice and assigning to plach — he's rumored to have magical bugsquashing powers in the multilingual area :)
@plach: the 3 remaining fails are all multilingual, and we have no idea why it's happening. Your insight would be most welcome :)
Comment #54
plachThe patch no longer applied: here's a plain reroll as I am seeing warnings all over the place. Let's see what the bot says.
Comment #55
plachNot sure what went wrong with the previous patch, but this one should have less warnings :)
Now I am looking at the test failures for reals...
Comment #59
plachThis should fix the multilingual search failures. Sorry for breaking other stuff, but I really have no time to work on core these days. I think this interdiff can be applied to a saner reroll to make everything green :)
However we might need more test coverage actually as the root cause was cache key not including language. Surprisingly test failed when run by an anonymous user (this is why locally they pass with the Simpletest UI), I am wondering whether that's somehow related to the role granularity.
Comment #61
plachCore moves fast these days...
Comment #63
plachAs you can see the SearchMultilingualEntities failures are gone, so the interdiff in #59 should be good :)
Comment #64
Wim LeersThank you SO much, plach! :)
In this reroll:
PageCacheTagsIntegrationTest.php
, which is a new test in latest D8 HEAD.I'm hopeful this one will be green — finally! :)
Comment #65
amateescu CreditAttribution: amateescu commentedThe last patch looks pretty good to me. I won't comment on the post_render_cache parts to not derail this issue, and I only found one thing that doesn't seem right:
Why are we excluding any other reference fields that might exist on a comment entity? See what the parent method does.
Comment #66
BerdirThis should include the parent/default implementation too? The comment might have an entity reference field and has an author and so on...
We might also want to add a todo to add a, possibly computed field that does expose the entity as a field, so that the generic code can deal with it, that's currently however blocked on #2114707: Allow per-bundle overrides of field definitions.
... Yeah, I can see that it too me too long to post this as i got distracted again, @amateescu basically said the same :)
Comment #67
Wim LeersOh, YES, great point #65/#66! Incredibly stupid of me!
Comment #69
Wim LeersStraight reroll. (Apparently, in the mean time HEAD had changed a
cache()
to\Drupal::cache()
.)Comment #70
jessebeach CreditAttribution: jessebeach commentedThis is a quick xhprof comparison of a single article (node/1) page load in a standard install. The first table is the page load, the second just the
drupal_render
function.Inclusive wall time improves significantly overall as one would expect with aggressive caching turned on. Exclusive wall time for
drupal_render
increases as, I assume, this function is now doing more work to assemble the cached output. Maybe, just a guess there :) But the increase is negligible.And we should be able to get these numbers even lower for a warm cache with #2099131: Use #pre_render pattern for entity render caching.
Comment #71
Wim LeersNICE! That's looking pretty good there! :) Let's also test what the effect is on a more complex node type: one with more fields. I think you'd also estimate/predict that the relative improvement should be even bigger, since the absolute time required should be pretty much exactly the same (i.e.: cache get).
It'd also be interesting to compare this to Drupal 7's numbers… :)
Unless somebody beats me to it, I'll do those comparisons.
Comment #72
moshe weitzman CreditAttribution: moshe weitzman commentedI reviewed this and found nothing to complain about. The performance gain is substantial.
Comment #73
webchickw00t w00t!! :) Sending over catch's way.
Comment #74
catchThe improvement is great but the overall numbers are horrible, three seconds for a simple node page is worse than Drupal 7 ever got iirc. At least this gives us a good opportunity to see what's left on a warm cache now, and there's still the #pre_render patch to go too.
I haven't reviewed latest patch yet, would be interesting to see Wim's numbers too but won't block a commit on it.
Comment #75
catchFew minor things.
Therefore.
whooo.
whoo.
How come?
And do we need to clear the node type above if we clearing all nodes?
Just assertTrue($nodes->length) no?
Same again.
Comment #76
jessebeach CreditAttribution: jessebeach commentedThis patch also needed a reroll.
Fixed.
Zero evaluates to false in PHP, so yes, the great than check is superfluous. Removed.
Agreed. Amaze. Such cache.
I do not know. Leaving to Wim to chime in. Hopefully he remembers whey this code was added.
Comment #78
jessebeach CreditAttribution: jessebeach commentedcache
can no longer be called as a global function.cache()
to\Drupal::cache()
Comment #79
Wim LeersRegarding
> 0
vs.assertTrue()
: I prefer explicitness. Asserting an integer evaluates to a boolean triggers the pedantic alarm in me. But of course it's equally correct.Regarding
+ \Drupal::entityManager()->getViewBuilder('node')->resetCache();
inNodeType.php
: amateescu added that in #15. I don't remember the specifics, but AFAICT it's there becauseNodeType != Node
, yetNodeType
affectsNode
. Therefor, a change in aNodeType
config entity (i.e. node entity bundle, e.g. "article") should cause the cache entries tagged with nodes of that type to be cleared. But we don't create cache tags for each node type (i.e. node entity bundle, e.g. "article"), therefor we must reset theNode
's view builder's cache completely.(I hope I didn't make any mistake in that reasoning — it's been a long day and I've been on the train for hours and still am.)
#78's interdiff looks good.
Back to RTBC since all nitpicks have been fixed, and I think/hope the answer above to the one question is sufficiently explanatory.
Am I misreading Jesse's numbers? Before the patch it's 4 seconds, after it's 1 second.
That's precisely why I wanted to do additional benchmarks, to get a sense of where we're at versus Drupal 7.
Yes! :)
I'm fine with this being committed. I agree additional profiling is necessary to get a more complete sense of where Drupal 8 is at node-rendering-performance-wise, but I think it's clear this is a step forward. Additional profiling is necessary in all corners of Drupal 8, so I don't think that should hold up a commit of this patch :)
Comment #80
Berdir@#70: Hm, not sure how you got those numbers, is it possible that the before is a cold start that needs to build up caches? There's no way that we could go down from 440k function calls to 86k just with this?
Did some runs myself, these are the numbers I came up with:
- node/1 with an article with a few tags, image, body and comment form.
- PHP 5.5 with opcache. Optimized MySQL and *fast* hardware.
- xhprof with only the memory flag (cpu is a huge overhead), enabled through devel.module, so we're missing the early bootstrap stuff, but that doesn't change anyway.
- I also had to patch xhprof to not track php functions as it segfaults otherwise on 5.5. I'm not sure how it exactly affects the results, probably at least the number of function calls is missing them, so I also did some good old ab tests to cross-check the numbers
- Logged in as uid 1
Full:
drupal_render()
There's a lot of post render cache stuff going on, so I closed comments and tested again.
Full:
drupal_render()
ab -n 100 -c 1 http://d8/1:
HEAD:
Patched:
=> 12% faster.
Numbers are pretty stable, I did multiple runs. As mentioned, xhprof wasn't covering the bootstrap part, so that should explain why those numbers are almost the same.
Because I usually can't stop when I start to profile stuff, here are some additional notes/experiments:
10% is spent in two calls to entity_create(), one due to EntityDisplayBase::getFieldDefinitions() and one in user_attach(). The first is going away in #2114707: Allow per-bundle overrides of field definitions, the second I'm not sure, can possibly be optimized to at least only run when we actually need it, it's only used as a fallback when a user can't be loaded.
Also, with a temporary hacked together combination of this, #597236: Add entity caching to core, #2182239: Improve ContentEntityBase::id() for better DX, #2114707: Allow per-bundle overrides of field definitions and removing the entity_create() in user_attach_accounts() I'm down to 118ms. One other thing is the mentioned pre_render() issue for running field formatters. While we certainly still have a lot to do, I have a feeling that Entity Field API is going to be pretty well optimized compared to to other parts at that point.
Need. to. stop. and. go. to. bed. :)
Comment #81
BerdirArg. Just noticed that I had xdebug + xhprof enabled, xhprof becomes much slower when xdebug is enabled. Relatively, the numbers should still be ok'ish, although fewer function calls might suggest more improvements than there actually are.
So just a few quick numbers, disabling xdebug, xhprof, devel, enabling apc classloader and then the same ab tests again:
HEAD:
With this patch:
With my fun issue mix-up from above, I'm getting this:
Before you get too excited about that, here's the same for D7 node/1:
So still 3x slower.
Comment #82
catchOK that looks a lot more like I was expecting on the profiling.
We should dsm() every comment submission that mentions xhprof with 'did you warm the cache both times and did you disable xdebug?' :P
The reasoning sounds right, we might want to revisit those very high level cache tags a bit, but since the patch only introduces the tag invalidation that's plenty for me.
Committed/pushed to 8.x, thanks!
Comment #83
Wim LeersWow — that's quite different. The numbers in #81 are … somewhat depressing, but definitely interesting. It'll be interesting to see where else we're going to find the gains we need to find.
Just to make sure: everything in #81 are authenticated user numbers, just like in #80, right?
Comment #84
Berdir#81/ab was done with anonymous users, but without page or block caching enabled.
Comment #85
chx CreditAttribution: chx commentedThis broke comment stubbing. #2207907: Comment stubbing is broken