Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove frameTable.implementation from the processed format. #5370

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Feb 14, 2025

Fixes #3713.

A frame's "implementation" was used to differentiate various JIT tiers. We now have subcategory information which fulfills this job more accurately.

So we can get rid of this extra data and reduce profile sizes.

This also lets us remove a lot of code.

I have a 3.1GB profile which contains 200MB of ,null for the implementation column. With this change that profile becomes 200MB smaller.


This PR leaves the Gecko format unchanged. As a follow-up we should write a Gecko patch and also bump the Gecko version, but there's no rush to do so; making this change only for the processed format achieves a large part of the benefits.

@mstange mstange requested a review from julienw February 14, 2025 03:58
@mstange mstange self-assigned this Feb 14, 2025
@mstange mstange changed the title Remove frameTable.implementation. Remove frameTable.implementation from the processed format. Feb 14, 2025
@mstange mstange force-pushed the remove-implementation branch from 45ab098 to 1ea2980 Compare February 14, 2025 04:04
@mstange mstange force-pushed the remove-implementation branch from 1ea2980 to b95d71b Compare February 14, 2025 04:08
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.95%. Comparing base (d8bba23) to head (b95d71b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5370      +/-   ##
==========================================
- Coverage   85.96%   85.95%   -0.01%     
==========================================
  Files         312      312              
  Lines       30331    30201     -130     
  Branches     8295     8249      -46     
==========================================
- Hits        26073    25959     -114     
+ Misses       3661     3645      -16     
  Partials      597      597              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks generally good, but please look at my comment below.
Also I remember we had some discrepencies between the implementation data vs the subcategory data, and I think we wanted to investigate where this comes from before removing them at all.
Here is an example I just captured: https://share.firefox.dev/42VYaHv
I think that the subcategory data is more precise but I also think that because it's diffently captured it might be wrong sometimes? I don't know and it may not be super important... But good to hear about your opinion on that!

Comment on lines +2376 to +2381
// The `implementation` column was removed from the frameTable. Modern
// profiles from Firefox use subcategories to represent the information
// about the JIT type of a JS frame.
for (const thread of profile.threads) {
delete thread.frameTable.implementation;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about converting this data into subcategory-based data, for these profiles that don't have subcategories.
But that's probably also very old profiles, so maybe not worth it.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:

production
deploy preview

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those profiles are old enough that people won't be interested in the JIT tiers anymore. But I'll take a look anyway, if it's really easy to migrate it may be worth doing.

@mstange
Copy link
Contributor Author

mstange commented Feb 14, 2025

I remember we had some discrepencies between the implementation data vs the subcategory data, and I think we wanted to investigate where this comes from before removing them at all. Here is an example I just captured: https://share.firefox.dev/42VYaHv

Sure, I can go through a few example stacks.

Link Implementation Subcategory
https://share.firefox.dev/3CHJb9E JS JIT (ion) JIT Compile (Baseline) Subcategory is more accurate; a function which is running in Ion is calling another function which is first getting baseline compiled.
https://share.firefox.dev/4b2oaTR JS JIT (baseline) JIT Compile (Ion) Subcategory is more accurate; a function which is running in Baseline is calling another function which is first getting Ion compiled.
https://share.firefox.dev/3CSNIWC JS JIT (blinterp) Parsing Subcategory is more accurate; a function which is running in Baseline-interpreter is calling a DOM method PrecompiledScript.executeInGlobal which needs to instantiate the parsed "stencil" representation of the executed script via js::frontend::InstantiateStencils.

I can look at more examples but the general pattern is the same: Subcategories are "finer". That's because subcategories can be set on profiler labels, whereas the implementation can only be set on JS function frames. And we have labels in more places than we have running JS functions.

And if I'm doing JS parsing during my Baseline JIT execution, I usually don't want to attribute that to "running Baseline JIT code" because the self time is not in the JIT code.

I think that the subcategory data is more precise but I also think that because it's diffently captured it might be wrong sometimes? I don't know and it may not be super important... But good to hear about your opinion on that!

I think it would be valid to say that the subcategory information is incomplete, for example there's no subcategory for time spent in Baseline ICs or in Ion ICs or in trampolines. But I can't think of a case where it's less correct than the "implementation" information.

Sometimes you do want to know "what's the JIT tier of the closest JS function further up the stack?" and the implementation answers that, mostly (unless it says "Native"). With subcategories you can answer this question by switching to the JS-only tree and by looking at the tooltip of the category square of the deepest JS function call node.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Re-evaluate whether we need the "implementation" concept, or if subcategories are enough
2 participants