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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: heycam, Assigned: heycam)

References

Details

Attachments

(2 files)

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.
Attached patch patchSplinter Review
Assignee: nobody → cam
Status: NEW → ASSIGNED
Attachment #8817097 - Flags: review?(xidorn+moz)
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)
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.
Flags: needinfo?(cam)
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)
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.
Do you think this works?
Flags: needinfo?(cam)
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)
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.
Yeah, I think you are right that this OK, since we do always cached reset structs on the style context.
Flags: needinfo?(cam)
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+
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
Looks like adding this code somehow blocks a layout flush. I don't immediately have idea why, though.
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9bb72b25ffb
Backed out changeset 3cf38f4d7395 for stylo test failure
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 :)
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.
(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.
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
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.
Well, it does make some sense given there's no style attribute apparently, huh.
(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.
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)
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)
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.
Attachment #8839514 - Flags: review+ → review?(cam)
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+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fefe9ce9ccc2
Fix CalcStyleDifference assumptions and PeekStyleContext semantics. r=heycam
https://hg.mozilla.org/mozilla-central/rev/fefe9ce9ccc2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.