1444296 - Include some traversal statistics in style tracing marker

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P1)

Description

[Xidorn Quan [:xidorn] UTC+10]

With parallel traversal, we don't include details of style threads in profile by default (and we don't want to either). This makes us completely blind on what's happening inside a restyles.

It may make sense to include some of statistics we are already collecting during traversal (as TraversalStatistics) into profile, so that we at least have some clue for long restyles in profile.

My feeling is that elements_{traversed,styled,matched} are probably the most interesting ones.

Comment 1

[Bobby Holley (:bholley)]

There is some small amount of overhead during the collection phase. Can we gate that on the profiler being enabled?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

I suspect gating them has a higher overhead than simply collecting them...

We should still gate them in the same way for merging the collected data, though.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #2)
> I suspect gating them has a higher overhead than simply collecting them...
> 
> We should still gate them in the same way for merging the collected data,
> though.

Yeah, that's what I meant. Sounds like we're on the same page.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1444296 part 1 - Have servo report traversal statistics to gecko.

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1444296 part 2 - Have a separate marker for styles to present stats.

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: perf.html pull request

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8958046 [details]
Bug 1444296 part 1 - Have servo report traversal statistics to gecko.

https://reviewboard.mozilla.org/r/226982/#review232824

I think we should fix up the statistics types to make things more clear.

Right now there are some stats that are gathered per-thread and then aggregated, whereas others are collected at the end via finish(). The type system doesn't enforce correctness here, making mistakes possible, and this patch makes the meaning of |finish| even less clear (since we don't call it for the |report| case).

So I think we should separate out the per-thread things into a TraversalStatisticsInner, and replace TraversalStatistics::finish() with a TraversalStatistics::new() that takes a TraversalStatisticsInner along with the other necessary bits. Then when we're reporting but not dumping, we can just collect and aggregated TraversalStatisticsInner instances, and then the dump code can construct a TraversalStatistics afterwards if it wants.

::: layout/style/ServoTraversalStatistics.h:26
(Diff revision 1)
> +  uint32_t mElementsMatched = 0;
> +  uint32_t mStylesShared = 0;
> +  uint32_t mStylesReused = 0;
> +
> +  static bool sActive;
> +  static ServoTraversalStatistics sStats;

Let's call this |singleton|.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Split TraversalStatistics into two parts.

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

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8958046 [details]
Bug 1444296 part 1 - Have servo report traversal statistics to gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226982/diff/1-2/

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8958047 [details]
Bug 1444296 part 2 - Have a separate marker for styles to present stats.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226984/diff/1-2/

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8958284 [details]
Split TraversalStatistics into two parts.

https://reviewboard.mozilla.org/r/227212/#review233018

This looks great, thanks. Please test that DUMP_STYLE_STATISTICS=1 still works on top of your patches.

r=me with that

::: servo/components/style/context.rs:328
(Diff revision 1)
> +        }
> +    }
> +}
> +
> +/// Statistics gathered during the traversal plus some other information
> +/// get from other sources including stylist.

s/get//

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8958046 [details]
Bug 1444296 part 1 - Have servo report traversal statistics to gecko.

https://reviewboard.mozilla.org/r/226982/#review233022

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Bobby Holley (:bholley) from comment #11)
> This looks great, thanks. Please test that DUMP_STYLE_STATISTICS=1 still
> works on top of your patches.

Yeah, it still works.

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8958047 [details]
Bug 1444296 part 2 - Have a separate marker for styles to present stats.

https://reviewboard.mozilla.org/r/226984/#review233588

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8958284 [details]
Split TraversalStatistics into two parts.

https://reviewboard.mozilla.org/r/227212/#review233600

::: servo/components/style/context.rs:314
(Diff revision 1)
>      /// StyleSharingCache.
>      pub styles_reused: u32,
> +}
> +
> +/// Implementation of Add to aggregate statistics across different threads.
> +impl<'a> ops::Add for &'a TraversalStatisticsInner {

Drive-by (sorry!), but... maybe `PerThreadTraversalStatistics`, calling the `inner` member something like `aggregated` instead?

Not ultra-fond of the name either, I just dislike the `FooInner` stuff a bit :).

Feel free to not change it if you don't think it's better / worth it.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8958051 [details] [review]
perf.html pull request

Julien has r+ed this in https://github.com/devtools-html/perf.html/pull/852 .

Comment 17

[Bobby Holley (:bholley)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> Drive-by (sorry!), but... maybe `PerThreadTraversalStatistics`, calling the
> `inner` member something like `aggregated` instead?

SGTM

Comment 18

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Servo PR

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8958046 [details]
Bug 1444296 part 1 - Have servo report traversal statistics to gecko.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226982/diff/2-3/

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8958047 [details]
Bug 1444296 part 2 - Have a separate marker for styles to present stats.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226984/diff/2-3/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab2df71dc9de
part 1 - Have servo report traversal statistics to gecko. r=bholley
https://hg.mozilla.org/integration/autoland/rev/7701560f1b14
part 2 - Have a separate marker for styles to present stats. r=mstange

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ab2df71dc9de
https://hg.mozilla.org/mozilla-central/rev/7701560f1b14