Closed Bug 816117 Opened 7 years ago Closed 7 years ago

Reduce Gecko Profiler overhead on windows

Categories

(Core :: Gecko Profiler, defect)

x86_64
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(3 files, 3 obsolete files)

At the moment the Gecko profiler has a very significant overhead on windows. We want to make that better.
Benoit, who else should review this?
Attachment #686129 - Flags: review?(bgirard)
Attachment #686131 - Attachment is obsolete: true
Attachment #686131 - Flags: review?(bgirard)
I can help with the reviews if you want.
Attachment #686129 - Flags: review?(bgirard) → review?(dbaron)
Comment on attachment 686130 [details] [diff] [review]
Part 2: Pass the thread context to NS_StackWalk on win32

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

r+ with this fixed

::: tools/profiler/TableTicker.cpp
@@ +796,5 @@
>    StackWalkCallback(aSample->pc, aSample->sp, &array);
>  
> +  void *platformData = nullptr;
> +#ifdef XP_WIN
> +  platformData = aSample->context;

Rather then having an ifdef here let's just intialize aSample->context on other platforms to nullptr and pass this directly to nsStackWalk.
Attachment #686130 - Flags: review?(bgirard) → review+
Attachment #686159 - Flags: review?(bgirard) → review+
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> I can help with the reviews if you want.

I took a look but feel free to drive-by review it.
(In reply to Benoit Girard (:BenWa) from comment #6)
> Comment on attachment 686130 [details] [diff] [review]
> Part 2: Pass the thread context to NS_StackWalk on win32
> 
> Review of attachment 686130 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with this fixed
> 
> ::: tools/profiler/TableTicker.cpp
> @@ +796,5 @@
> >    StackWalkCallback(aSample->pc, aSample->sp, &array);
> >  
> > +  void *platformData = nullptr;
> > +#ifdef XP_WIN
> > +  platformData = aSample->context;
> 
> Rather then having an ifdef here let's just intialize aSample->context on
> other platforms to nullptr and pass this directly to nsStackWalk.

As discussed on IRC you can disregard this because the context is used differently on linux.
(In reply to comment #7)
> (In reply to Ehsan Akhgari [:ehsan] from comment #5)
> > I can help with the reviews if you want.
> 
> I took a look but feel free to drive-by review it.

Nah, I meant if you don't wanna review yourself.  ;-)
What does -  mFontSizeInflationForceEnabled = nsLayoutUtils::FontSizeInflationForceEnabled(); have to do with stack walking?
(In reply to Taras Glek (:taras) from comment #10)
> What does -  mFontSizeInflationForceEnabled =
> nsLayoutUtils::FontSizeInflationForceEnabled(); have to do with stack
> walking?

....

I have no idea where that line came from, I have no idea what it does.. but let's ignore that change and pretend it isn't there :)
Duplicate of this bug: 805815
review ping. I'd like to get this patch landed before we scale up startup/shutdown profiling.
Comment on attachment 686129 [details] [diff] [review]
Part 1: Add the ability to pass around a windows ThreadContext to StackWalkMain64

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

::: layout/base/nsPresShell.cpp
@@ -9166,5 @@
>  {
>    mFontSizeInflationEmPerLine = nsLayoutUtils::FontSizeInflationEmPerLine();
>    mFontSizeInflationMinTwips = nsLayoutUtils::FontSizeInflationMinTwips();
>    mFontSizeInflationLineThreshold = nsLayoutUtils::FontSizeInflationLineThreshold();
> -  mFontSizeInflationForceEnabled = nsLayoutUtils::FontSizeInflationForceEnabled();

To mention the obvious, please put this back in.  :-)

::: xpcom/base/nsStackWalk.cpp
@@ +305,5 @@
>      BOOL ok;
>  
>      // Get a context for the specified thread.
> +    if (!data->platformData) {
> +      memset(&context, 0, sizeof(CONTEXT));

Nit: please maintain the 4-space indentation to match the rest of the file.

::: xpcom/base/nsStackWalk.h
@@ +37,5 @@
>   *                     current thread. On Windows, this is a thread HANDLE.
>   *                     It is currently not supported on any other platform.
> + * @param aPlatformData Platform specific data that can help in walking the
> + *                      stack, this should be NULL unless you really know
> + *                      what you're doing!

Please document that this needs to be a CONTEXT* on Windows, and should not be passed on other platforms.
Attachment #686129 - Flags: review?(dbaron) → review+
Update to allow nsTraceAlloc.c to be compiled.
Attachment #686129 - Attachment is obsolete: true
Attachment #689749 - Flags: review?(ehsan)
Correct patch this time.
Attachment #689749 - Attachment is obsolete: true
Attachment #689749 - Flags: review?(ehsan)
Attachment #689750 - Flags: review?(ehsan)
Comment on attachment 689750 [details] [diff] [review]
Part 1: Add the ability to pass around a windows ThreadContext to StackWalkMain64 v3

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

Let's not use a default arg in C++ either, and just update all callers to pass in a NULL/nullptr...  :/
Attachment #689750 - Flags: review?(ehsan) → review-
(and r=me on a patch which does that!)
https://hg.mozilla.org/mozilla-central/rev/89dca2ab7d86
https://hg.mozilla.org/mozilla-central/rev/cdae5d81c8e3
https://hg.mozilla.org/mozilla-central/rev/33c70f306fd4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.