Bobby Holley (:bholley) Reporter
	Description • 8 years ago

Per discussion in bug 1331578, it would be useful to have systems of collecting performance statistics on both the gecko and servo style systems, so that we can compare them in automation and track regressions.

As a basic first pass, I think we should track the number of elements for which we perform selector matching. Other suggestions?

If an environmental variable is set, we'll dump the collected statistics to the console at relevant points. In Gecko, this would happen after frame construction and after processing restyles. In Servo it can just happen at the end of the style traversal.

Ideally the statistic collection would be cheap enough that we can do it unconditionally. In the parallel Servo traversal, this probably means storing the counters on the ThreadLocalStyleContext, and then aggregating them after the join.

	Bobby Holley (:bholley) Reporter
	Comment 1 • 8 years ago

Shing, any opinions on what kind of logging format we want here? It's possible that a given testcase may log several times (if, for example, it performs the initial load, and then some JS causes an incremental restyle). So it'd be best if the harness could aggregate the statistics across several dumps for a given page.

	Shing Lyu [:shinglyu]
	Comment 2 • 8 years ago

For the performance test I'm running, I'm using the following format: https://github.com/servo/servo/blob/master/etc/ci/performance/test_runner.py#L14

[PERF] perf block start
[PERF],restyle_count,999
[PERF],other_stuff,1
[PERF] perf block end

The `[PERF]` prefix is used to filter out the line from the rest of the log. I also added a `perf block start` and `perf block end` line to wrap around the numbers, so we can get the results block by block.

	Shing Lyu [:shinglyu]
	Comment 3 • 8 years ago

Using a line-based logging format is better than JSON or XML, because sometimes there will be other modules that spits logs in between your structural data. e.g.

```
{
  "restyle_count": 999,
WARNING: some other log from other module << breaks the JSON
}
```

	Bobby Holley (:bholley) Reporter
	Comment 4 • 8 years ago

This is the servo piece. I'll do the Gecko piece tomorrow.

MozReview-Commit-ID: ECHZgYNA8nb

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 8 years ago

Comment on attachment 8828225 [details] [diff] [review]
Add style performance statistics to Servo. v1

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

Looks good to me.

One question: Should we add the restyle generation to the traversal statistics?

Not a huge deal, but probably worth so we can try to test it?

Other way to test this stuff would be to expose it as a DOM API so we can write tests. It should be easy on Gecko at least, but probably ok to do as a follwup.

::: servo/components/style/parallel.rs
@@ +76,5 @@
> +    // Dump statistics to stdout if requested.
> +    if TraversalStatistics::should_dump() || opts::get().style_sharing_stats {
> +        let slots = unsafe { tls.unsafe_get() };
> +        let aggregate = slots.iter().fold(TraversalStatistics::default(), |acc, t| {
> +            match *(t.borrow()) {

nit: Unnecessary parens?

	Emilio Cobos Álvarez (:emilio)
	Comment 6 • 8 years ago

Comment on attachment 8828225 [details] [diff] [review]
Add style performance statistics to Servo. v1

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

::: servo/components/style/context.rs
@@ +115,5 @@
> +    pub elements_styled: u32,
> +    /// The number of elements for which we performed selector matching.
> +    pub elements_matched: u32,
> +    /// The number of cache hits from the StyleSharingCache.
> +    pub styles_shared: u32,

After reading bug 1331848 comment 5, I think we could try to keep the reason of the style sharing failure here too (would be an array indexed by the style sharing's CacheMiss, but you can also do that as a followup I guess.

	Bobby Holley (:bholley) Reporter
	Comment 7 • 8 years ago

(In reply to Emilio Cobos Álvarez [:emilio] from comment #5)
> One question: Should we add the restyle generation to the traversal
> statistics?

We removed the restyle generation, remember? ;-)

> Other way to test this stuff would be to expose it as a DOM API so we can
> write tests. It should be easy on Gecko at least, but probably ok to do as a
> follwup.
> 
> ::: servo/components/style/parallel.rs
> @@ +76,5 @@
> > +    // Dump statistics to stdout if requested.
> > +    if TraversalStatistics::should_dump() || opts::get().style_sharing_stats {
> > +        let slots = unsafe { tls.unsafe_get() };
> > +        let aggregate = slots.iter().fold(TraversalStatistics::default(), |acc, t| {
> > +            match *(t.borrow()) {
> 
> nit: Unnecessary parens?

Fixed.


(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> Comment on attachment 8828225 [details] [diff] [review]
> Add style performance statistics to Servo. v1
> 
> Review of attachment 8828225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: servo/components/style/context.rs
> @@ +115,5 @@
> > +    pub elements_styled: u32,
> > +    /// The number of elements for which we performed selector matching.
> > +    pub elements_matched: u32,
> > +    /// The number of cache hits from the StyleSharingCache.
> > +    pub styles_shared: u32,
> 
> After reading bug 1331848 comment 5, I think we could try to keep the reason
> of the style sharing failure here too (would be an array indexed by the
> style sharing's CacheMiss, but you can also do that as a followup I guess.

I think that would make more sense as logging. The main statistic we'd want to surface is "the cache isn't working", and then the person debugging it can just turn on the logging.

	Bobby Holley (:bholley) Reporter
	Comment 8 • 8 years ago

The Servo stats landed in https://github.com/servo/servo/pull/15119

	Bobby Holley (:bholley) Reporter
	Comment 9 • 8 years ago

Thinking about it a bit more, I'm not totally convinced that we need the Gecko statistics right now. It's kind of harder to track them given the piecemeal way which styles are resolved, so I'm going to de-scope this bug for now and file a new one if it seems worthwhile.