Closed Bug 1326134 Opened 3 years ago Closed 3 years ago

Record profiling-relevant information in profiles

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

It would be nice if profiles recorded whether various performance-sensitive settings (eg DEBUG) were enabled for a given profile, to avoid wild goose chases.
This reports whether we're spending time poisoning freed space.
Attachment #8822300 - Flags: review?(jcoppeard)
Record whether DEBUG was enabled, and also asking your opinion of how far to push this -- should we somehow pass through whether --enable-optimize was given? I figured we should only do really important ones, so we're not always chasing after new ones (and having to decide what to do if it isn't present in a profile.)
Attachment #8822301 - Flags: review?(mstange)
Nick, this is marked f? because I couldn't figure how this is applied or used. Apparently enabling async stacks can affect perf by quite a bit. But I couldn't find anywhere in the code that looked at this pref, other than what looked like a test. And I'm not sure what the default should be. Please edumacate me.
Attachment #8822302 - Flags: feedback?(nfitzgerald)
Comment on attachment 8822301 [details] [diff] [review]
Report DEBUG builds in profiles

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

What's the use case here? Displaying a warning in the profiler UI?

::: tools/profiler/core/GeckoSampler.cpp
@@ +355,5 @@
> +  aWriter.IntProperty("debug-build", 1);
> +#else
> +  aWriter.IntProperty("debug-build", 0);
> +#endif
> +  

end-of-line whitespace
Attachment #8822301 - Flags: review?(mstange) → review+
Comment on attachment 8822302 [details] [diff] [review]
Report whether javascript.options.asyncstack is active

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

It makes every DOM callback creation and Promise creation capture and hold a stack that we then link when running the callback or promise's then-functions, to chain the original stack across the "async gap". Yes, this is fairly expensive, despite some work that went into making it less so. It should probably be disabled unless devtools are open (except the profiler tool), but others have pushed back against that. /me shrugs
Attachment #8822302 - Flags: feedback?(nfitzgerald) → feedback+
(In reply to Markus Stange [:mstange] from comment #4)
> Comment on attachment 8822301 [details] [diff] [review]
> Report DEBUG builds in profiles
> 
> Review of attachment 8822301 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What's the use case here? Displaying a warning in the profiler UI?

For things that have a high probability of making profiling results invalid, yes. (DEBUG would be in that category.) I have seen more than one person spend a bunch of time poring over a profile, optimizing various bits, only to eventually realize that they're profiling a DEBUG build and so were looking at completely irrelevant areas of the code.

For other things, I was imagining that they would just be available when you looked at some sort of profile summary. The old Cleopatra had such a thing (with other info like the sampling frequency). I don't see it in the new. Perhaps the Summary tab could have it? I'm not sure it's important enough to highlight there. Anyway, the desire is to have a place to look to better understand the profile you're looking at -- when it was captured, what sort of build it was, maybe a simplified form of the category summaries. Hm, now that I say that, perhaps the Summary tab *is* the right place for it. (I think the smoothed category average display probably ought to be shown elsewhere anyway, preferably somewhere in the portion where the X axis represents time, and replaced in the Summary tab with a very basic top 3 category percentages or something.)
Ok. I agree on all counts.
I'm tempted to add MOZ_OPTIMIZE_FLAGS in as well, but maybe that's too much detail.
Attachment #8822557 - Flags: review?(mh+mozilla)
Ok, I tried it out, and it appears that this *does* do the right thing. And I tracked through the code and found where this is read. It all makes sense now.
Attachment #8822560 - Flags: review?(nfitzgerald)
Attachment #8822302 - Attachment is obsolete: true
Attachment #8822560 - Flags: review?(nfitzgerald) → review+
Blocks: 1298117
Comment on attachment 8822300 [details] [diff] [review]
Report whether GC poisoning is enabled in profiles

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

Good idea.
Attachment #8822300 - Flags: review?(jcoppeard) → review+
Landing what is reviewed now, marking leave-open. This bug can be closed when all attached patches land; I don't have any more things I intend to add. Yet.
Keywords: leave-open
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3abf2766315
Report whether GC poisoning is enabled in profiles, r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf6b85c31fd5
Report DEBUG builds in profiles, r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/074fb90d914d
Report whether javascript.options.asyncstack is active, r=fitzgen
Comment on attachment 8822557 [details] [diff] [review]
Report optimization level

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

I'm tempted to say this is useless. Many people are using e.g. --enable-optimize=-O0 to disable optimizations, and guess what that does? Yeah, that sets MOZ_OPTIMIZE to 1.
So, in fact, MOZ_OPTIMIZE_FLAGS would be a little better, but not necessarily much, because there are so many ways to enable optimizations that it's not funny.
Note that GCC defines __OPTIMIZE__ when optimization is enabled (and it's *not* defined when -O0 is passed), which sounds like a much better thing to check for... it looks like clang does have that too, now the question is whether msvc has something similar.

::: tools/profiler/moz.build
@@ +118,5 @@
>  
>      if CONFIG['MOZ_WIDGET_TOOLKIT'] == 'gonk' and (CONFIG['ANDROID_VERSION'] <= '17' or CONFIG['ANDROID_VERSION'] >= '21'):
>          DEFINES['ELFSIZE'] = 32
>  
> +    DEFINES['MOZ_OPTIMIZE'] = CONFIG.get('MOZ_OPTIMIZE') or 0

CONFIG.get('MOZ_OPTIMIZE', 0)
Attachment #8822557 - Flags: review?(mh+mozilla)
(In reply to Mike Hommey [:glandium] from comment #14)
> > +    DEFINES['MOZ_OPTIMIZE'] = CONFIG.get('MOZ_OPTIMIZE') or 0
> 
> CONFIG.get('MOZ_OPTIMIZE', 0)

This is for people who "unset" MOZ_OPTIMIZE by setting it to the empty string.

> Note that GCC defines __OPTIMIZE__ when optimization is enabled (and it's
> *not* defined when -O0 is passed), which sounds like a much better thing to
> check for... it looks like clang does have that too, now the question is
> whether msvc has something similar.

That sounds way better. But from my searching, it doesn't look like MSVC has an equivalent. I could fabricate something. But perhaps I should just give up on capturing any sort of ill-defined "optimized" flag.
I have sort of wanted to report optimization level from the binary for other reasons now, but I'm going to mark this bug RESOLVED now that everything else is in. I'll open up another bug if I decide to do something about opt.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.