Closed
Bug 1322317
Opened 7 years ago
Closed 7 years ago
stylo: we are calling FinishStyle on some style structs from style worker threads
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(2 files)
3.79 KB,
patch
|
Details | Diff | Splinter Review | |
59 bytes,
text/x-review-board-request
|
heycam
:
review+
|
Details |
In some situations we end up calling FinishStyle, which does the main thread work for a newly computed style struct, on style worker threads. I think this is due to PeekStyleXXX calls on the current (old) style context within CalcStyleDifference. Under PeekStyleXXX, we never compute a style struct, only return an existing one, so we shouldn't need to call FinishStyle on it.
Assignee | ||
Comment 1•7 years ago
|
||
Comment 2•7 years ago
|
||
This patch makes me think we probably should call FinishStyle eagerly, e.g. somehow when creating the corresponding StyleContext, rather than lazily in StyleXXX(). WDYT?
Flags: needinfo?(cam)
Comment 3•7 years ago
|
||
I talked with heycam in the work week about this, and he thought that if we do that eagerly, we may end up calling it much more times than we need. I think it is in general not an issue, since structs contain URL are in general not inherited, so resolved is only called on limited node which actually have those value specified. But there are two structs which have URL and inherited, which are for cursor and list-style-image.
Updated•7 years ago
|
Flags: needinfo?(cam)
Comment 4•7 years ago
|
||
Comment on attachment 8817097 [details] [diff] [review] patch Review of attachment 8817097 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleContext.h @@ +642,5 @@ > } else { \ > newData = \ > Servo_GetStyle##name_(mSource.AsServoComputedValues()); \ > } \ > + if (aComputeData) { \ This isn't necessary. if aComputeData is false, this method should have returned from above.
Attachment #8817097 -
Flags: review?(xidorn+moz)
Comment 5•7 years ago
|
||
And given that we always cache computed data in nsStyleContext for stylo, I think we should early return from methods for reset structs as well when aComputeData is false.
Comment 7•7 years ago
|
||
I hit this today. We no longer call FinishStyle from CalcStyleDifference for inherit structs, but that's still not true for reset structs. I wonder if we can just return nullptr if aComputeData is false for reset structs too. Assertion failure: NS_IsMainThread(), at /home/emilio/projects/moz/stylo/layout/style/nsStyleStruct.cpp:1169 #01: nsStyleSVGReset::FinishStyle(nsPresContext*) (/home/emilio/projects/moz/stylo/layout/style/nsStyleStruct.cpp:1169 (discriminator 1)) #02: nsStyleSVGReset const* nsStyleContext::DoGetStyleSVGReset<false>() (/home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:139 (discriminator 5)) #03: nsStyleContext::PeekStyleSVGReset() (/home/emilio/projects/moz/stylo/obj-x86_64-pc-linux-gnu/layout/style/nsStyleStructList.h:139) #04: nsChangeHint nsStyleContext::CalcStyleDifferenceInternal<FakeStyleContext>(FakeStyleContext*, nsChangeHint, unsigned int*, unsigned int*) (/home/emilio/projects/moz/stylo/layout/style/nsStyleContext.cpp:1088 (discriminator 21)) #05: nsStyleContext::CalcStyleDifference(ServoComputedValues const*, nsChangeHint, unsigned int*, unsigned int*) (/home/emilio/projects/moz/stylo/layout/style/nsStyleContext.cpp:1273) #06: Gecko_CalcStyleDifference (/home/emilio/projects/moz/stylo/layout/style/ServoBindings.cpp:315) #07: style::gecko::restyle_damage::{{impl}}::compute (/home/emilio/projects/moz/stylo/servo/components/style/gecko/restyle_damage.rs:53) #08: style::matching::MatchMethods::compute_restyle_damage<style::gecko::wrapper::GeckoElement> (/home/emilio/projects/moz/stylo/servo/components/style/matching.rs:987) #09: style::matching::PrivateMatchMethods::accumulate_damage<style::gecko::wrapper::GeckoElement> (/home/emilio/projects/moz/stylo/servo/components/style/matching.rs:636) #10: style::matching::PrivateMatchMethods::cascade_primary_or_pseudo<style::gecko::wrapper::GeckoElement> (/home/emilio/projects/moz/stylo/servo/components/style/matching.rs:603) #11: style::matching::MatchMethods::cascade_element<style::gecko::wrapper::GeckoElement> (/home/emilio/projects/moz/stylo/servo/components/style/matching.rs:1023) #12: style::traversal::compute_style<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/emilio/projects/moz/stylo/servo/components/style/traversal.rs:551) #13: style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/emilio/projects/moz/stylo/servo/components/style/traversal.rs:428) #14: style::gecko::traversal::{{impl}}::process_preorder (/home/emilio/projects/moz/stylo/servo/components/style/gecko/traversal.rs:45) #15: style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:116) #16: style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:143) #17: style::parallel::traverse_nodes<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:143) #18: style::parallel::traverse_dom::{{closure}}::{{closure}}<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (/home/emilio/projects/moz/stylo/servo/components/style/parallel.rs:77) #19: rayon::scope::{{impl}}::execute_job_closure::{{closure}}<closure> (/home/emilio/projects/moz/stylo/third_party/rust/rayon/src/scope/mod.rs:314) #20: std::panic::{{impl}}::call_once<(),closure> (/checkout/src/libstd/panic.rs:296) #21: std::panicking::try::do_call<std::panic::AssertUnwindSafe<closure>,()> (/checkout/src/libstd/panicking.rs:450) #22: panic_abort::__rust_maybe_catch_panic (/checkout/src/libpanic_abort/lib.rs:42)
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
I'm hitting this all the time locally after some changes for display: contents. I can't think of any reason for which we couldn't return null there, since if we'd computed it already it'd be cached on the context (since we cache it on the context all the time). Let me know if I'm missing something.
Assignee | ||
Comment 10•7 years ago
|
||
Yeah, I think you are right that this OK, since we do always cached reset structs on the style context.
Flags: needinfo?(cam)
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8839514 [details] Bug 1322317: Fix CalcStyleDifference assumptions and PeekStyleContext semantics. https://reviewboard.mozilla.org/r/114128/#review115792
Attachment #8839514 -
Flags: review?(cam) → review+
Comment 12•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3cf38f4d7395 Don't call FinishStyle off-main-thread for reset structs. r=heycam
Comment 13•7 years ago
|
||
Backed this out for stylo test failure like https://treeherder.mozilla.org/logviewer.html#?job_id=79283194&repo=autoland&lineNumber=1781
Comment 14•7 years ago
|
||
Looks like adding this code somehow blocks a layout flush. I don't immediately have idea why, though.
Comment 15•7 years ago
|
||
Backout by ihsiao@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e9bb72b25ffb Backed out changeset 3cf38f4d7395 for stylo test failure
Comment 16•7 years ago
|
||
Huh, weird. This is the failure that I thought my display: contents changes caused and was investigating (bug 1341083 comment 12). Well, this saved me a few minutes of bisecting uselessly, but now I need to find the cause of this :)
Comment 17•7 years ago
|
||
Also, note that while debugging above, I noticed that if you comment all the test cases except the height one, it passes, which makes it even funnier to debug.
Comment 18•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17) > Also, note that while debugging above, I noticed that if you comment all the > test cases except the height one, it passes, which makes it even funnier to > debug. Which makes me look wary at our eRestyle_StyleAttribute handling, let's see.
Comment 19•7 years ago
|
||
I've been taking a look to this. Not sure why this change exactly happens to trigger that test failure, but our handling of the style attribute seems to be not working properly (in that test with all the test cases commented except the text-shadow and the height one, at the end of the test the content claims to have "text-shadow: 30px 40px yellow" as the style attribute, which seems wrong. huh
Comment 20•7 years ago
|
||
So my previous comment is wrong. We're just generating an empty change hint, which means we haven't used the StylePosition struct, which makes no sense at all.
Comment 21•7 years ago
|
||
Well, it does make some sense given there's no style attribute apparently, huh.
Comment 22•7 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #21) > Well, it does make some sense given there's no style attribute apparently, > huh. And of course I'm wrong again. I have no idea why PeekStylePosition is returning null in CalcStyleDifference.
Comment 23•7 years ago
|
||
Well, of course I just came back, and I think I know what's going on. So the main difference between how Gecko and Servo do style differences is that Servo doesn't preserve the computed style structs when doing it. That is, when we do CalcStyleDifference, we don't compute (and thus cache) the structs that the old style had. If they haven't changed and are not needed to handle the change of style that happened before, those structs are now just lost in the void. For example, in this case we have a div, that to position itself has had to compute the StylePosition struct. After that, we change its text-shadow, which means that we have a style change where position doesn't change. With this patch, calling PeekStylePosition _won't_ cache the struct, so it won't be compared again. This is sad, since it really feels like a hack. I wonder if we can just avoid calling FinishStyle when aComputeData is false, and return the struct pointer. But that is suboptimal at least. Perhaps we could store in the restyle data both the change hint and the computed structs? It'd be an easy thing to add to FakeStyleContext. We'd then compute the structs right after calling Servo_TakeChangeHint. Bobby, Cam, does that seem reasonable? An alternative would be keeping doing what we do but deferring the FinishStyle call to the main thread, but that seems roughly the same amount of complexity, and we lose that optimization (which may or may not matter, but given the similar amount of complexity...).
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
Comment 24•7 years ago
|
||
Emilio and I discussed this, and here's what I think we should do: (1) Stop using the style struct pointers on nsStyleContext entirely. Always just fetch from Servo via FFI. (2) Use the style context bits to determine whether Peek returns true or false. (3) In RecreateStyleContexts, copy over the relevant bits from the old style context. Call FinishStyle for any of those bits that are set. (4) Fix the bug where PeekFoo() currently does the same as Foo().
Flags: needinfo?(cam)
Flags: needinfo?(bobbyholley)
Comment 25•7 years ago
|
||
Need to run now, but I just scheduled a try run with a patch that seems to work (https://treeherder.mozilla.org/#/jobs?repo=try&revision=d54ad3e6e8b9bf6b0599bb5fb65dbbfedd52dc66). If it's green I'll clean them up when I'm back.
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8839514 -
Flags: review+ → review?(cam)
Assignee | ||
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8839514 [details] Bug 1322317: Fix CalcStyleDifference assumptions and PeekStyleContext semantics. https://reviewboard.mozilla.org/r/114128/#review116602 MozReview thinks I've already r+ed this, but r=me with these things addressed, and an answer to the aComputeData change. Also, since we now should never fill in any of the mCachedInheritedData / mCachedResetData struct pointers for a Servo-backed nsStyleContext, can you add a few assertions around the place: * at the top of SetStyle, that we aren't Servo-backed * in ~nsStyleContext, that if we are Servo-backed, then mCachedInheritedData must only contain nulls, and mCachedResetData must be null, and then we can skip that whole "// Free up our data structs." block. (Actually we could already have been skipping this, but may as well do that now.) ::: layout/style/nsStyleContext.h:96 (Diff revision 2) > nsIAtom* aPseudoTag, > mozilla::CSSPseudoElementType aPseudoType, > already_AddRefed<ServoComputedValues> aComputedValues, > bool aSkipParentDisplayBasedStyleFixup); > > + Nit: don't need this new line? ::: layout/style/nsStyleContext.h:672 (Diff revision 2) > + * assertion to debug them. \ > + */ \ > + if (mPseudoTag == nsCSSAnonBoxes::mozText) { \ > + MOZ_ASSERT(mParent); \ > + const nsStyle##name_* data = \ > + mParent->DoGetStyle##name_<aComputeData>(); \ What's the consequence of this change from using <true> to <aComputeData>? ::: layout/style/nsStyleContext.h:688 (Diff revision 2) > + \ > + const nsStyle##name_* data = \ > + Servo_GetStyle##name_(mSource.AsServoComputedValues()); \ > + /* perform any remaining main thread work on the struct */ \ > + if (needToCompute) { \ > + AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_); \ I think the AUTO_CHECK_DEPENDENCY assertions don't really check anything useful for stylo, since all of the struct computations have already been done on the Servo side. So I think you can just remove it from the Servo branch here. ::: layout/style/nsStyleContext.cpp:182 (Diff revision 2) > +void > +nsStyleContext::EnsureStructsForServo(const nsStyleContext& aOldContext) Ah, this is just like a function I've got in my bug 1302054 part 1 patch (but aimed at Gecko). :-) Nit: Other functions that take nsStyleContext objects as arguments do so as pointers, so I think we should do the same here rather than use a reference. ::: layout/style/nsStyleContext.cpp:188 (Diff revision 2) > +#define STYLE_STRUCT_INHERITED(name_, checkdata_cb) \ > + if (aOldContext.mBits & NS_STYLE_INHERIT_BIT(name_)) { \ > + mozilla::Unused << Style##name_(); \ > + } > + > +#define STYLE_STRUCT_RESET STYLE_STRUCT_INHERITED You can just do: #define STYLE_STRUCT(name_, checkdata_cb_) ... to capture inherited and reset structs at the same time. ::: layout/style/nsStyleContext.cpp:204 (Diff revision 2) > + "Should have at least as many structs computed as the \ > + old context!"); This doesn't do the same thing as in rust, where leading white space will be removed. Instead use two string literals: "Should have at least as many structs computed as the " "old context!");
Attachment #8839514 -
Flags: review?(cam) → review+
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/fefe9ce9ccc2 Fix CalcStyleDifference assumptions and PeekStyleContext semantics. r=heycam
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fefe9ce9ccc2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•