support Gecko profiler for stylo

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
11 months ago
5 months ago

People

(Reporter: heycam, Assigned: froydnj)

Tracking

({leave-open})

unspecified
leave-open
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

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.
Priority: -- → P3
We also might need to register the style worker threads with the profiler.

Updated

10 months ago
Blocks: 1324668
Blocks: 1330412
No longer blocks: 1243581
(Assignee)

Comment 2

10 months ago
I would like to investigate this.  Chris, do you know if anybody else is working on this?
Flags: needinfo?(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.
Assignee: nobody → nfroyd
Flags: needinfo?(cpeterson)
(Assignee)

Comment 4

8 months ago
Opened https://github.com/nikomatsakis/rayon/issues/225 to discuss adding the necessary support to rayon.
No longer blocks: 1330412
Summary: stylo: support Gecko profiler → support Gecko profiler for stylo
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.
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
Flags: needinfo?(nmatsakis)
(Assignee)

Comment 7

7 months ago
Created attachment 8855339 [details] [diff] [review]
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.
Attachment #8855339 - Flags: review?(xidorn+moz)
Niko just released rayon 0.7:

https://github.com/nikomatsakis/rayon/releases/tag/v0.7.0
Flags: needinfo?(nmatsakis)
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.
Attachment #8855339 - Flags: review?(xidorn+moz) → review+
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?
(Assignee)

Comment 11

7 months ago
(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?
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

7 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d24b3983c4c2
add servo bindings for profiler thread registration; r=xidorn
(Assignee)

Updated

7 months ago
Keywords: leave-open
sorry had to back this out causing conflicts merging inbound to m-c like  for warning: conflicts while merging layout/style/ServoBindings.cpp
Flags: needinfo?(nfroyd)

Comment 15

7 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5d7a2c00b6
Backed out changeset d24b3983c4c2 for causing merge conflict with m-c

Comment 16

7 months ago
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3146ac58280a
add servo bindings for profiler thread registration; r=xidorn
(Assignee)

Updated

7 months ago
Flags: needinfo?(nfroyd)

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3146ac58280a
Is there more to do here?
Flags: needinfo?(nfroyd)
(Assignee)

Comment 19

6 months ago
(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.
Flags: needinfo?(nfroyd)
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
(Assignee)

Comment 21

6 months ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
: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.
(Assignee)

Comment 23

5 months ago
(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?
Flags: needinfo?(gtatum)
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
Flags: needinfo?(gtatum)
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
And in particular, it sure would be nice to coalesce those worker threads!
You need to log in before you can comment on or make changes to this bug.