Closed
Bug 1180120
Opened 9 years ago
Closed 9 years ago
don't restyle children if we have a non-inherited property change and no child explicitly inherits or uses it
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: heycam, Assigned: heycam)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(9 files, 1 obsolete file)
5.14 KB,
patch
|
dbaron
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.90 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
4.56 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
12.60 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
4.73 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
3.46 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
4.00 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
11.81 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
38.93 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
In bug 931668 we made the restyling process stop as it traverses down the tree if we can be sure that the styles of descendant frames won't change from a given point. This still can end up restyling the children of a frame that only has a non-inherited property change, to check whether their styles are the same. For example with: <div> ... 1000 <p> elements ... </div> if we set div.style.opacity = 0.5 then we'll still restyle all 1000 <p> children to check whether we can cut off the restyling after them. If we track whether a given style context explicitly inherited a non-inherited property (or otherwise used it), we could stop the restyling process immediately after restyling the <div>. See bug 1136893 for a proof of concept patch and some comments.
Assignee | ||
Comment 1•9 years ago
|
||
Explicitly inheriting non-inherited properties is rare, so I think we can just have a single bit that tracks whether any non-inherited property was explicitly inherited, rather than trying to track individual properties.
Assignee | ||
Comment 2•9 years ago
|
||
I was hoping this would help with bug 1172239 but I realise (of course) that the same conditions that make it safe to return eRestyle_Stop on children are the same conditions we'd need to check on those children from the parent, if returning this new restyle hint. Because the children of the element we change the non-inherited property on in that page have shared style contexts, we can't stop restyling.
Assignee | ||
Comment 3•9 years ago
|
||
I realised that the set of conditions is smaller when considering whether to stop at the element itself, so I have some patches that are mostly working now. https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e101f2e477 -- couple of test failures still.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8641012 -
Flags: review?(dbaron)
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8641013 -
Flags: review?(dbaron)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8641014 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8641015 -
Flags: review?(dbaron)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8641016 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8641018 -
Flags: review?(dbaron)
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8641019 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5eac3b90c5c4
Assignee | ||
Comment 12•9 years ago
|
||
One of the XXXs I left in Part 7 was about whether we need to worry about ::before/::after frame creation in MoveStyleContextsForChildren. I'm pretty sure now we don't have to check for them, since we would have got an eRestyle_Subtree in that case. (Leaving these reviewless until dbaron can accept reviews again.)
Attachment #8641019 -
Attachment is obsolete: true
Attachment #8641019 -
Flags: review?(dbaron)
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Comment 15•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6545de5925b2
Comment on attachment 8641012 [details] [diff] [review] Part 1: Record on style contexts that reset style is explicitly inherited. I think I reviewed a patch in another bug (maybe 1172087) that uses 0x400000000, so be careful when merging. I think it's worth filing a followup on making setting this bit more exact. But if you're getting useful wins from just this optimization, then this seems worth landing. >+ /* This is pessimistic; we could be uncacheable because we relied on */ \ >+ /* the parent's font-size, for example, which does not need to */ \ Would that even fall into this case, or would it hit conditions.Cacheable() ? r=dbaron
Attachment #8641012 -
Flags: review?(dbaron) → review+
Comment on attachment 8641013 [details] [diff] [review] Part 2: Move style-if-visited as well in nsStyleContext::Move, if its parent is the same as the non-visited-style parent. >+ aNewParent->AddRef(); >+ mStyleIfVisited->mParent->RemoveChild(mStyleIfVisited); >+ mStyleIfVisited->mParent = aNewParent; >+ mStyleIfVisited->mParent->AddChild(mStyleIfVisited); >+ oldParent->Release(); You should probably either: (1) set oldParent = mStyleIfVisited->mParent before this chunk and use oldParent on the second of the quoted lines, or (2) use mStyleIfVisited->mParent instead of oldParent on the fifth of the quoted lines (I know you assert they're the same, but...) >Bug 1180120 - Part 2: Move style-if-visited as well in nsStyleContext::Move, if its parent is the same as the non-visited-style parent. The commit message's use of "if" is a bit odd. You're asserting that that, not testing it. I'm not sure I see where this is going, though. Are you planning to change ElementRestyler::ComputeRestyleResultFromNewContext so that we execute this code? (Or am I wrong in thinking the first early return there prevents us from doing so?) r=dbaron
Attachment #8641013 -
Flags: review?(dbaron) → review+
Comment on attachment 8641014 [details] [diff] [review] Part 3: Record on ElementRestyler whether we are at the root of a restyle. Maybe mRootOfRestyle instead of just mRoot? (Or mIsRootOfRestyle?) r=dbaron with that
Attachment #8641014 -
Flags: review?(dbaron) → review+
Comment on attachment 8641015 [details] [diff] [review] Part 4: Add an outparam to CalcStyleDifference to indicate which struct pointers were identical. >+ // We check for struct pointer equality here rather than as part of the >+ // DO_STRUCT_DIFFERENCE calls, since those calls can result in structs >+ // we previously examined and found to be null on this style context >+ // getting computed by later DO_STRUCT_DIFFERENCE calls (which can >+ // happen when the nsRuleNode::ComputeXXXData method looks up another >+ // struct.) Could you extend the comment to briefly (one sentence?) explain why you want the after-state rather than the before-state? r=dbaron
Attachment #8641015 -
Flags: review?(dbaron) → review+
Attachment #8641016 -
Flags: review?(dbaron) → review+
Comment on attachment 8641018 [details] [diff] [review] Part 6: Replace HasSameCachedStyleData call with samePointerStructs bit tests. >+ // XXX This loop could be rewritten as bit operations on oldContext->mBits Maybe FIXME rather than XXX? (and then rewrap)
Attachment #8641018 -
Flags: review?(dbaron) → review+
Attachment #8642221 -
Flags: review+
Comment on attachment 8642222 [details] [diff] [review] Part 6.2: Refactor MaybeReframeFor{Before,After}Pseudo. I feel like these used to be more different in terms of the performance characteristics of getting before and after frames being different; it looks like Mats changed that in bug 907396 (changeset bf4849f9b835). (I worry that I may have made them more different again in bug 1110277 patch 3 (changeset 1bbebe9fec17).) So I guess this is now a valid refactoring. The before and after cases previously different in whether they passed mContent (::after) or aContent (::before) to AppendChange. Your code passes aContent, which matches ::before. I can't figure out any cases when mContent and aContent would be different, though. r=dbaron
Attachment #8642222 -
Flags: review+
Comment on attachment 8642223 [details] [diff] [review] Part 7: Add eRestyleResult_StopWithStyleChange. (v2) nsStyleStructFwd.h: >+// Bits for inherited structs. >+#define NS_STYLE_INHERITED_STRUCT_MASK \ >+ ((1 << nsStyleStructID_Inherited_Count) - 1) >+// Bits for reset structs. >+#define NS_STYLE_RESET_STRUCT_MASK \ >+ (((1 << nsStyleStructID_Reset_Count) - 1) \ >+ << nsStyleStructID_Inherited_Count) This has a slightly hidden implicit dependency on nsStyleStructID being 32-bit (or, in particular, the same size as an integer literal 1). Maybe cast the 1 to nsStyleStructID, if that compiles? (Or maybe declare an nsStyleStructID_size_t like nsRestyleHint_size_t, etc.) >+ // XXX Bail out if we have out-of-flow or placeholder frames; not sure if it's >+ // safe to just skip them. Do you need to do this? And also the other XXX comments? (Maybe this one actually isn't ready for review? I thought you didn't request it only because I wasn't accepting reviews.)
Assignee | ||
Comment 23•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC-4 (busy Aug. 8-Aug. 30) from comment #22) > Comment on attachment 8642223 [details] [diff] [review] > Part 7: Add eRestyleResult_StopWithStyleChange. (v2) ... > >+ // XXX Bail out if we have out-of-flow or placeholder frames; not sure if it's > >+ // safe to just skip them. > > Do you need to do this? And also the other XXX comments? The XXX applies to the "not sure if it's safe to just skip them", which I don't know the answer to still. For the HasChildThatUsesGrandancestorStyle XXX, I hadn't looked into whether that check indeed was needed. However this check will disappear once (if) we refcount style structs, which is why I didn't bother thinking too hard about it. For the undisplayed content XXX, I just didn't bother with adding a method analagous to RestyleUndisplayedContent, as my main focus was getting something that worked for the Amazon page. Still, we should handle this (now or soon).
Flags: needinfo?(dbaron)
Flags: needinfo?(dbaron)
Attachment #8642223 -
Flags: review?(dbaron)
Comment on attachment 8642223 [details] [diff] [review] Part 7: Add eRestyleResult_StopWithStyleChange. (v2) >+ mRestyleTracker.AddRestyleRootsIfAwaitingRestyle(descendants); >+ if (aRestyleHint & eRestyle_SomeDescendants) { >+ AddPendingRestylesForDescendantsMatchingSelectors(mContent); >+ } I should perhaps come back to this bit another time, since I haven't digested that patch (which I believe bz reviewed). But for now I'll trust you on this being what's needed, although you should be aware that I didn't review this bit well. Why does RestyleSelf not process struct swaps when it returns StopWithStyleChange, such that if we can't Stop, we need to process them later? I don't understand that bit -- and it seems like it could at least use a comment. In ComputeRestyleResultFromFrame: >+ // need to check antthing. s/antthing/anything/ (and same in ComputeRestyleResultFromNewContext) > if (oldContext->HasChildThatUsesGrandancestorStyle()) { > LOG_RESTYLE_CONTINUE("the old context uses grandancestor style"); >- return eRestyleResult_Continue; >+ aRestyleResult = eRestyleResult_Continue; >+ aCanStopWithStyleChange = false; >+ return; > } I wonder if aCanStopWithStyleChange is really needed here -- would the bit that says a child inherits has a non-inherited be set? This probably doesn't need to be fixed now, but maybe worth a followup if that's the case? (If it is, would need to drop the early return.) RestyleSelf: >+ // If we have to restyle descendants, we can't return >+ // eRestyleResult_StopWithStyleChange. >+ if (aRestyleHint & (eRestyle_Subtree | eRestyle_LaterSiblings | >+ eRestyle_Force | eRestyle_ForceDescendants)) { Comment should probably say "children" rather than "descendants" given the inclusion of "Force", which only implies children and not other descendants. It's also not clear why LaterSiblings needs to be there. Is it something to do with struct swapping? MoveStyleContextsForChildren: >+ // XXX Bail out if there are undisplayed or display:contents children. >+ // We could get this to work if we need to. This should probably be written as: // Bail out if there are undisplayed or display:contents children. // FIXME: We could get this to work if we need to. since the "XXX" is currently confusing. I think it's worth filing a followup bug at least for undisplayed children; they're reasonably common. >+ if (MustCheckUndisplayedContent(undisplayedParent)) { >+ nsCSSFrameConstructor* fc = mPresContext->FrameConstructor(); >+ if (fc->GetAllUndisplayedContentIn(undisplayedParent) || >+ fc->GetAllDisplayContentsIn(undisplayedParent)) { I don't see the MustCheckUndisplayedContent method in my tree -- not sure what introduces it -- but is it correct that you're conditioning the display:contents check on it? MoveStyleContextsForContentChildren: >+ // XXX Bail out if we have out-of-flow or placeholder frames; not sure if it's >+ // safe to just skip them. Please split this in half and move it below to where you do it, with the FIXME before the part that's to be fixed. >+ if (child->GetStateBits() & NS_FRAME_OUT_OF_FLOW) { >+ return false; >+ } I think it should be safe to just skip out-of-flows. Their style contexts come from someplace else. So I think this could just be a continue. (Although I'm ok fixing that in a followup if you're worried about it. But I think it should be fine.) Skipping placeholders for now seems reasonable, though. (That's where their style contexts come from.) Maybe file a followup bug on not skipping placeholders, though? r=dbaron with that Sorry about the delay getting to this. We should probably try to get this uplifted to both aurora and beta.
Attachment #8642223 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy Aug. 8-Aug. 30) from comment #16) > Comment on attachment 8641012 [details] [diff] [review] > Part 1: Record on style contexts that reset style is explicitly inherited. ... > >+ /* This is pessimistic; we could be uncacheable because we relied on */ \ > >+ /* the parent's font-size, for example, which does not need to */ \ > > Would that even fall into this case, or would it hit conditions.Cacheable() ? If it's relying on the parent's font-size, then it would be uncacheable. If it relies on the current element's font-size, then it's cacheable with dependencies. Might be better to use a different example, though.
Assignee | ||
Comment 26•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy Aug. 8-Aug. 30) from comment #17) > Comment on attachment 8641013 [details] [diff] [review] > Part 2: Move style-if-visited as well in nsStyleContext::Move, if its parent > is the same as the non-visited-style parent. ... > I'm not sure I see where this is going, though. Are you planning > to change ElementRestyler::ComputeRestyleResultFromNewContext so that > we execute this code? (Or am I wrong in thinking the first early > return there prevents us from doing so?) You would've eventually seen, but it gets invoked by ElementRestyler::MoveStyleContextsForChildren in part 7.
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy Aug. 8-Aug. 30) from comment #24) > Comment on attachment 8642223 [details] [diff] [review] > Part 7: Add eRestyleResult_StopWithStyleChange. (v2) ... > Why does RestyleSelf not process struct swaps when it returns > StopWithStyleChange, such that if we can't Stop, we need to process them > later? I don't understand that bit -- and it seems like it could at least > use a comment. Are you asking why we can't we just do the struct swaps unconditionally when returning StopWithStyleChange? It's not necessary to do the swap -- we have checked that the only difference in cached struct pointers is the reset structs that have changed. > > if (oldContext->HasChildThatUsesGrandancestorStyle()) { > > LOG_RESTYLE_CONTINUE("the old context uses grandancestor style"); > >- return eRestyleResult_Continue; > >+ aRestyleResult = eRestyleResult_Continue; > >+ aCanStopWithStyleChange = false; > >+ return; > > } > > I wonder if aCanStopWithStyleChange is really needed here -- would > the bit that says a child inherits has a non-inherited be set? This > probably doesn't need to be fixed now, but maybe worth a followup if > that's the case? (If it is, would need to drop the early return.) I'd have to think a bit more deeply to see if this one can be dropped. Once we do shared style structs we probably won't need to track grandancestor-style-usage anyway. > RestyleSelf: > > > >+ // If we have to restyle descendants, we can't return > >+ // eRestyleResult_StopWithStyleChange. > >+ if (aRestyleHint & (eRestyle_Subtree | eRestyle_LaterSiblings | > >+ eRestyle_Force | eRestyle_ForceDescendants)) { > > Comment should probably say "children" rather than "descendants" given > the inclusion of "Force", which only implies children and not other > descendants. > > It's also not clear why LaterSiblings needs to be there. Is it something > to do with struct swapping? I think it can be removed. Will verify. > >+ if (MustCheckUndisplayedContent(undisplayedParent)) { > >+ nsCSSFrameConstructor* fc = mPresContext->FrameConstructor(); > >+ if (fc->GetAllUndisplayedContentIn(undisplayedParent) || > >+ fc->GetAllDisplayContentsIn(undisplayedParent)) { > > I don't see the MustCheckUndisplayedContent method in my tree -- not sure > what introduces it -- but is it correct that you're conditioning the > display:contents check on it? Yes.
Assignee | ||
Comment 28•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3eab8797d64
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/88e5e1fc929a https://hg.mozilla.org/integration/mozilla-inbound/rev/4dac8ab1e1a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8a95346a08 https://hg.mozilla.org/integration/mozilla-inbound/rev/6229c2f38ea9 https://hg.mozilla.org/integration/mozilla-inbound/rev/129945037746 https://hg.mozilla.org/integration/mozilla-inbound/rev/bcb20dbe84bd https://hg.mozilla.org/integration/mozilla-inbound/rev/f8942f7f4edd https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed938d9bb87 https://hg.mozilla.org/integration/mozilla-inbound/rev/25b33fb7dc51
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88e5e1fc929a https://hg.mozilla.org/mozilla-central/rev/4dac8ab1e1a7 https://hg.mozilla.org/mozilla-central/rev/ed8a95346a08 https://hg.mozilla.org/mozilla-central/rev/6229c2f38ea9 https://hg.mozilla.org/mozilla-central/rev/129945037746 https://hg.mozilla.org/mozilla-central/rev/bcb20dbe84bd https://hg.mozilla.org/mozilla-central/rev/f8942f7f4edd https://hg.mozilla.org/mozilla-central/rev/1ed938d9bb87 https://hg.mozilla.org/mozilla-central/rev/25b33fb7dc51
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee | ||
Comment 31•9 years ago
|
||
Comment on attachment 8641012 [details] [diff] [review] Part 1: Record on style contexts that reset style is explicitly inherited. (applies to all patches on the bug) Approval Request Comment [Feature/regressing bug #]: N/A [User impact if declined]: very poor performance on amazon.com (bug 1172239) [Describe test coverage new/current, TreeHerder]: patches been on m-c for a week and a bit, manual testing confirmed it (along with other dependencies of bug 1172239) solved the performance issue [Risks and why]: moderate; restyle code is complex, and risks include incorrectly styled content if the patches are buggy [String/UUID change made/needed]: N/A
Attachment #8641012 -
Flags: approval-mozilla-aurora?
Comment 32•9 years ago
|
||
Comment on attachment 8641012 [details] [diff] [review] Part 1: Record on style contexts that reset style is explicitly inherited. We want to fix that for 42, let's take it.
Attachment #8641012 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/d1fbc7d972ed https://hg.mozilla.org/releases/mozilla-aurora/rev/f5eb4b908add https://hg.mozilla.org/releases/mozilla-aurora/rev/07d4bad39bc4 https://hg.mozilla.org/releases/mozilla-aurora/rev/dbf8d7d2b935 https://hg.mozilla.org/releases/mozilla-aurora/rev/7fca6840789d https://hg.mozilla.org/releases/mozilla-aurora/rev/6ddcf355f740 https://hg.mozilla.org/releases/mozilla-aurora/rev/80143eb3ab73 https://hg.mozilla.org/releases/mozilla-aurora/rev/121de382cd52 https://hg.mozilla.org/releases/mozilla-aurora/rev/124b3fa18283
Depends on: 1230386
Depends on: 1340593
You need to log in
before you can comment on or make changes to this bug.
Description
•