1398119 - NoteDirtyElement is either called too often or is too slow or both

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Olli Pettay [:smaug][[e-mail]]]

NoteDirtyElement is 4.6% of innerHTML set time in a profile I'm looking at.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

https://perfht.ml/2eLtOhP

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

innerHTML speed is rather important for Speedometer.

Comment 3

[Bobby Holley (:bholley)]

So 3/4 samples are in MaybeConstructLazily, so the work involved here is basically propagating up the lazy frame construction bits, which Gecko does too. Stylo's code here is slightly more complex than Gecko's, but would be faster in a lot of cases (the first time we set the bit we don't need to walk up the tree, whereas Gecko always walks up to the root).

How does the time under MaybeConstructLazily compare between stylo and non-stylo?

Comment 4

[Emilio Cobos Álvarez (:emilio)]

I took a look, and we're mostly taking the fast paths there, so unless the `parent` checks are hurting us (in which case we could at least move the aElement == existingRoot up), I'm not sure there's much we can do here.

In my profile NoteDirtyElement was also incorrectly blamed with some refcount traffic, that really cam from BindToTree / UnbindFromTree, for some reason.

The other case where we may pay more than Gecko is in propagating the HAS_DIRTY_DESCENDANTS bits, which we could try to coalesce in MaybeConstructLazily.

I'll post a patch with the first idea, please take a profile if you wish, because mine was kinda busted because of the refcount traffic that was incorrectly reported.

I can also try to poke at (2), I guess...

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1398119: Move the common case of NoteDirtyElement during innerHTML up.

Review commit: https://reviewboard.mozilla.org/r/177790/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177790/

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

stylo vs non-stylo isn't too interesting question here. We are spending too much time in a _very_ hot code path (and doesn't matter whether it is stylo or non-stylo, but in this case it happens to be stylo).
nsIMutationObserver implementations should be as fast as possible.
Can we do something to optimize the method, or call it less often, or call it asynchronously?

Comment 7

[Bobby Holley (:bholley)]

Realistically, assuming we aren't taking a regression with the switch to stylo, I probably don't have time to think about this until after the 57 merge.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Oh, sure, I don't think I indicated anywhere this _must_ go into FF57.
It is just that we have a slow method in a very hot code path.

I may take a look at this.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Let me profile without stylo

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

The stuff under nsCSSFrameConstructor::ContentAppended seems to take more time when Stylo is enabled, 
but doesn't affect bug 1347525 too badly.
Note, this seems to be a case where Gecko profiler may not give low-level enough information, so some native profiler may be better.

Comment 11

[Andrew Overholt [:overholt]]

Now that 57 is shipped, let's re-evaluate if we can do work here. I'm marking this qf:p1 but am willing to hear objections :)

Comment 12

[Andrew Overholt [:overholt]]

smaug said he'd profile this again to see where things are these days.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Ok, I was reprofiling this using Nightly, and I can't see any high numbers for NoteDirtyElement anymore.
Marking this WFM. Please reopen if needed.
(FWIW, I'm getting quite a bit faster innerHTML numbers for Nightly than for Chrome, on Linux.)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Reopening. Using a different testcase the method is there.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

https://perfht.ml/2D34Gkk

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

https://perfht.ml/2D1L6os might be a bit better link

Comment 17

[Bobby Holley (:bholley)]

I can have a look at this testcase.

Comment 18

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3d408b40e63cd2710987a99e9e0fd49653ddcc3

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add machinery to assert single-threadedness from geckolib. v1

MozReview-Commit-ID: 9LBNm2h5Ct3

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 2 - Avoid atomic overhead in Servo_Element_IsDisplayNone. v1

This reduces time spent in NoteDirtyElement by about 15% in the testcase. Even
though the subsequent patch will cause us to call Servo_Element_IsDisplayNone less
for this particular testcase, it could still be hot on other testcases, and so
it's worth optimizing.

MozReview-Commit-ID: 3F3Zfp48dDW

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 3 - Rearrange NoteDirtyElement for faster bailouts on the same restyle root. v1

This eliminates ~90% of the time spent in NoteDirtyElement on this testcase.

MozReview-Commit-ID: Lm5hf7QRiOK

Comment 22

[Bobby Holley (:bholley)]

(Note that I bumped the iterations on the testcase to get better resolution of the effects of these patches)

Comment 23

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add machinery to assert single-threadedness from geckolib. v2

MozReview-Commit-ID: 9LBNm2h5Ct3

Comment 24

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c674de9a8abb5351aacd6888356b7b474c58ba1

Comment 25

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=321e423cf82decde6c249b8787d0fa179d3fc081

Comment 26

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add machinery to assert single-threadedness from geckolib. v3

MozReview-Commit-ID: 9LBNm2h5Ct3

Comment 27

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8942022 [details] [diff] [review]
Part 1 - Add machinery to assert single-threadedness from geckolib. v3

Review of attachment 8942022 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with those allows removed.

::: servo/ports/geckolib/glue.rs
@@ +215,5 @@
>  unsafe fn dummy_url_data() -> &'static RefPtr<URLExtraData> {
>      RefPtr::from_ptr_ref(&DUMMY_URL_DATA)
>  }
>  
> +#[allow(dead_code)]

Why is this needed? This shouldn't warn even in release builds.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941993 [details] [diff] [review]
Part 2 - Avoid atomic overhead in Servo_Element_IsDisplayNone. v1

Review of attachment 8941993 [details] [diff] [review]:
-----------------------------------------------------------------

I'd rather not take it if it happens to not be needed with the next patch tbh. But looks good if it is indeed needed...

Comment 29

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8941994 [details] [diff] [review]
Part 3 - Rearrange NoteDirtyElement for faster bailouts on the same restyle root. v1

Review of attachment 8941994 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIDocumentInlines.h
@@ +97,5 @@
> +nsIDocument::SetServoRestyleRootDirtyBits(uint32_t aDirtyBits)
> +{
> +  MOZ_ASSERT(aDirtyBits);
> +  MOZ_ASSERT((aDirtyBits & ~Element::kAllServoDescendantBits) == 0);
> +  MOZ_ASSERT((aDirtyBits & mServoRestyleRootDirtyBits) == mServoRestyleRootDirtyBits);

Let's assert mServoRestyleRoot here?

Comment 30

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #27)
> Comment on attachment 8942022 [details] [diff] [review]
> Part 1 - Add machinery to assert single-threadedness from geckolib. v3
> 
> Review of attachment 8942022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with those allows removed.
> 
> ::: servo/ports/geckolib/glue.rs
> @@ +215,5 @@
> >  unsafe fn dummy_url_data() -> &'static RefPtr<URLExtraData> {
> >      RefPtr::from_ptr_ref(&DUMMY_URL_DATA)
> >  }
> >  
> > +#[allow(dead_code)]
> 
> Why is this needed? This shouldn't warn even in release builds.

It does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd55825e8b0ab26262e2dbe59962dbb55d375caf&selectedJob=155779839

This is because is_in_servo_traversal is currently only used in a #[cfg(not(debug_asssertions))] block. I could remove it for is_main_thread, but that would be asymmetric, and also likely to bite people again in the future if code gets rearranged. The advantage of deny(dead_code) is that it makes sure that stuff gets cleaned up when it's not used, but I don't think that's a particularly high priority for these functions.

(In reply to Emilio Cobos Álvarez [:emilio] from comment #28)
> I'd rather not take it if it happens to not be needed with the next patch
> tbh. But looks good if it is indeed needed...

Per the commit message, it depends on the testcase. Part 3 further optimizes this particular testcase so that Servo_Element_IsDisplayNone will never be reached, but it will on other testcases.

In general I think it's fine to bypass locks in main-thread-only glue.rs FFI hooks because:
* The locks are designed to protect us from racing with rayon, but the assertions ensure that rayon can't be running at these callsites.
* We're one frame away from C++ code, so we don't get deep transitive benefits of type-safety. The only reason we're calling into Rust at all is that the data representation is opaque to C++, and we we wouldn't dream of paying this kind of runtime cost in C++.

That said, I also don't think we should sprinkle safety bypasses willy-nilly, even in glue.rs. So I think a good compromise is "we can bypass if we can measure a clear speedup in a profiler".

(In reply to Emilio Cobos Álvarez [:emilio] from comment #29)
> Let's assert mServoRestyleRoot here?

Good idea.

Thanks for the reviews!

Comment 31

[Bobby Holley (:bholley)]

https://github.com/servo/servo/pull/19753

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af26f7cb29ca
Add machinery to assert single-threadedness from geckolib. r=emilio
https://hg.mozilla.org/integration/autoland/rev/dae099622091
Rearrange NoteDirtyElement for faster bailouts on the same restyle root. r=emilio

Comment 33

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/af26f7cb29ca
https://hg.mozilla.org/mozilla-central/rev/dae099622091