1322656 - support Gecko profiler for stylo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Cameron McCormack (:heycam)]

We have some calls to the Gecko profiler in classes such as nsStyleSet and ElementRestyler (which add labels) that won't be run when stylo is enabled.  We need to do something equivalent from the Rust side when we are using stylo.

Comment 1

[Cameron McCormack (:heycam)]

We also might need to register the style worker threads with the profiler.

Comment 2

[Nathan Froyd [:froydnj]]

I would like to investigate this.  Chris, do you know if anybody else is working on this?

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> I would like to investigate this.  Chris, do you know if anybody else is
> working on this?

Thanks! No one is looking at the profiler yet, but profiler support will be important once we start getting Hasal performance reports.

Comment 4

[Nathan Froyd [:froydnj]]

Opened https://github.com/nikomatsakis/rayon/issues/225 to discuss adding the necessary support to rayon.

Comment 5

[Bobby Holley (:bholley)]

Just FYI that I'm approaching the point where I'd like to start profiling the parallel traversal. Still probably a few days off but would be nice to get the threads registered.

Comment 6

[Chris Peterson [:cpeterson]]

Niko, do you have an ETA of when rayon 0.7 will be released? You just merged Nathan's rayon thread startup/shutdown changes [1] that necessary to add Rust support to the Gecko Profiler.

If there are other post-v0.6 rayon changes that still need stabilization before you can release 0.7, would you consider a cutting a 0.7 release that is just 0.6 plus Nathan's thread startup/shutdown changes? This will be a big help for the Stylo team's profiling efforts for Quantum.

[1] https://github.com/nikomatsakis/rayon/issues/225

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: add servo bindings for profiler thread registration

We need these to be able to register parallel traversal threads with the
profiler.

Getting the Gecko side stuff ready to go so perhaps the servo-side stuff will
be easier.

Comment 8

[Chris Peterson [:cpeterson]]

Niko just released rayon 0.7:

https://github.com/nikomatsakis/rayon/releases/tag/v0.7.0

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8855339 [details] [diff] [review]
add servo bindings for profiler thread registration

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

I'm not familiar with profiler at all, but this patch looks reasonable and simple enough, so rs=me.

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

It seems to me there is a mutex in profiler_register_thread unconditionally hold. Would that be a problem given we may spawn several threads at once?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #10)
> It seems to me there is a mutex in profiler_register_thread unconditionally
> hold. Would that be a problem given we may spawn several threads at once?

We can only register one thread at a time; it's a bit problematic in that some threads might start work in the thread pool before others have even spawned, but I don't think it will be a problem in practice?  We could add a barrier to ensure all the threads have started and registered before they start work, though?

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Oh, hmm, so we have a thread pool for that, I guess that's fine then. I thought that we spawn threads for every tree traversal, where the additional hook could take time. But if we reuse threads a lot, that wouldn't be a problem then.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24b3983c4c2
add servo bindings for profiler thread registration; r=xidorn

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out causing conflicts merging inbound to m-c like  for warning: conflicts while merging layout/style/ServoBindings.cpp

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5d7a2c00b6
Backed out changeset d24b3983c4c2 for causing merge conflict with m-c

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3146ac58280a
add servo bindings for profiler thread registration; r=xidorn

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3146ac58280a

Comment 18

[Bobby Holley (:bholley)]

Is there more to do here?

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #18)
> Is there more to do here?

There are two things left to do on the servo side:

* Integrate rayon 0.7: https://github.com/servo/servo/pull/16303  This is blocked by Mac test timeouts.  My Mac seems incapable of running servo atm, so I have a new machine ordered, but if you knew of someone who could debug these, that would be great!
* Add patches to servo that use the new rayon APIs.  I have this patch written, it's just waiting on rayon.

Comment 20

[Chris Peterson [:cpeterson]]

rayon 0.7 has been merged to servo master and mozilla-autoland:

https://github.com/servo/servo/commit/fc3a7d03f2b0c5292e7414e7555fb75ae0b1d663

https://hg.mozilla.org/integration/autoland/rev/62536f85ab9c226b5997cb07e7f10e164b5db8f1

Comment 21

[Nathan Froyd [:froydnj]]

https://github.com/servo/servo/pull/16786 has landed on autoland, so stylo threads should be able to be seen by the Gecko Profiler.  You'll have to manually add them to the list of threads the Gecko Profiler is following.  I'll send a message to stylo-team after autoland merges to m-c (probably tomorrow morning my time) with an announcement and what you need to do.

Comment 22

[Greg Tatum [:gregtatum]]

:froydnj could you or someone share a few profiles from perf.html of stylo here so that I can take a look at what those profiles look like? It'll be helpful for working on the performance client. We have a few open issues I'd like to include them on.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Greg Tatum [:gregtatum] [@gregtatum] from comment #22)
> :froydnj could you or someone share a few profiles from perf.html of stylo
> here so that I can take a look at what those profiles look like? It'll be
> helpful for working on the performance client. We have a few open issues I'd
> like to include them on.

You just want a profile with the Stylo threads active (and ideally doing something)?  I'm curious what you expect to get out of such profiles that a semi-random profile wouldn't already give you?

Comment 24

[Greg Tatum [:gregtatum]]

I like working from real data when working with features. I have a request to support merging of threads in perf.html. It would be nice when developing a feature to see if the resulting functionality was sensible with real data.

https://github.com/devtools-html/perf.html/issues/88

Comment 25

[Bobby Holley (:bholley)]

Here's a profile I took of [1]: https://perfht.ml/2qrbely

Notice that filtering the content process main thread on Servo_TraverseSubtree will show you the segments where the main thread kicks over to the parallel workers.

[1] https://en.wikipedia.org/wiki/Barack_Obama

Comment 26

[Bobby Holley (:bholley)]

And in particular, it sure would be nice to coalesce those worker threads!

Comment 27

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.