1752861 - Set the flag "relevant for js" on existing label frames and add new ones for JavaScript developers

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement, P3)

Description

[Julien Wajsberg [:julienw]]

I noticed some label frames could be useful to show in the "javascript" tree, such as "Style Computation". Then thanks to Markus I realized that a full class of label frames could be interesting to show: the ones using their subcategory as their label. Indeed these labels should be human readable and most of the time interesting to javascript developers.

We can try to turn them on all first. If the result is that we have too many label unrelevant label frames, we could come up with different macros for these cases.

Comment 1

[Julien Wajsberg [:julienw]]

Here is a profile example: https://share.firefox.dev/3ALx5qo

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1752861 - [profiler] Set the flag 'relevant for js' on label frames that have the flag 'LABEL_DETERMINED_BY_CATEGORY_PAIR' r=gerald

Comment 3

[Julien Wajsberg [:julienw]]

After discussion we need to be more fine-grained, therefore come up with new macros so that some of them would be relevant for JS but others wouldn't.

Comment 4

[Julien Wajsberg [:julienw]]

Here are the main "blocks" that web developers expect to see: JavaScript / Style / Layout / Paint / Composite.
(From https://developers.google.com/web/fundamentals/performance/rendering)

We could have different labels and maybe a few more blocks (or rather sub-parts of them) but this should be as simple.

Comment 5

[Julien Wajsberg [:julienw]]

Here is an example of a profile with the new patches: https://share.firefox.dev/3KkgbTc
This is a profile of the profiler UI.
As a comparison, the same scenario on the current Nightly: https://share.firefox.dev/3ty1WDx

Another example on loading a page on leboncoin.fr:

• with the patch: https://share.firefox.dev/3KbW1uG
• with Nightly: https://share.firefox.dev/3HLYFpk

My approach has been that I didn't want any category to disappear from the activity graph or the flame chart, focusing on when this is useful for web developers. For example I added a frame label for GC happening in Web page but not for the GC happening after clicking on the button in about:memory. It's likely that I missed some cases that we'll be able to add later too.

Comment 6

[Julien Wajsberg [:julienw]]

Here is a try build with these changes: https://treeherder.mozilla.org/jobs?repo=try&revision=0f1aaa33e73fba52c0d8f84373a6fa96a43758c4

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: Bug 1752861 - [profiler] Add a few new macros to add frame labels that will show up in javascript stacks r=gerald

Comment 8

[Julien Wajsberg [:julienw]]

Attachment: Bug 1752861 - [profiler] Rename and expose to JS some existing frame labels as well as adding a few r=gerald!,mstange!

Depends on D140388

Comment 9

[Julien Wajsberg [:julienw]]

Attachment: Bug 1752861 - [profiler] Expose GC and CC operations to the JS view r=gerald!,sfink!

Depends on D140389

Comment 10

[Julien Wajsberg [:julienw]]

I'm gonna add a few more things later too.

Comment 11

[Julien Wajsberg [:julienw]]

Comment on attachment 9266574 [details]
Bug 1752861 - [profiler] Expose GC and CC operations to the JS view r=gerald!,sfink!

Beta/Release Uplift Approval Request

• User impact if declined: These patches add useful information to JavaScript developers using the profiler. It would be good to have that in beta earlier.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This doesn't change the behavior for users that don't use the profiler.
  For users of the profiler, this is adding some information and removing other, but nothing is risky here.
• String changes made/needed:

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8e224b84b277
[profiler] Add a few new macros to add frame labels that will show up in javascript stacks r=gerald
https://hg.mozilla.org/integration/autoland/rev/90ef854bcaf7
[profiler] Rename and expose to JS some existing frame labels as well as adding a few r=mstange
https://hg.mozilla.org/integration/autoland/rev/16794e0d61b7
[profiler] Expose GC and CC operations to the JS view r=sfink,smaug

Comment 13

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/8e224b84b277
https://hg.mozilla.org/mozilla-central/rev/90ef854bcaf7
https://hg.mozilla.org/mozilla-central/rev/16794e0d61b7

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9266572 [details]
Bug 1752861 - [profiler] Add a few new macros to add frame labels that will show up in javascript stacks r=gerald

Approved for 99.0b4. Thanks.

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9266573 [details]
Bug 1752861 - [profiler] Rename and expose to JS some existing frame labels as well as adding a few r=gerald!,mstange!

Approved for 99.0b4. Thanks.

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9266574 [details]
Bug 1752861 - [profiler] Expose GC and CC operations to the JS view r=gerald!,sfink!

Approved for 99.0b4. Thanks.

Comment 17

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f15a12d887e9
https://hg.mozilla.org/releases/mozilla-beta/rev/7aae08bfe15d
https://hg.mozilla.org/releases/mozilla-beta/rev/34afd8162b88

Comment 18

[Donal Meehan [:dmeehan]]

:julienw - could this bug be closed and used to only track the uplifts?
Track any follow-up work in separate bugs.

Comment 19

[Julien Wajsberg [:julienw]]

Sure, let's do that. Thanks for the feedback!