927979 - Register the gecko thread correctly in the profiler for FxMetro

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

See https://bugzilla.mozilla.org/show_bug.cgi?id=886555#c20 as well as the next few comments. To get more useful profiles on Metro we need to register the gecko thread with the profiler correctly, which currently doesn't happen.

Comment 1

[Jim Mathies [:jimm]]

(In reply to Benoit Girard (:BenWa) from comment #24)
> I think kats is going to handle it. But we need to add these calls when the
> thread is started and ended:
> http://mxr.mozilla.org/mozilla-central/source/tools/profiler/GeckoProfiler.
> h#162
> 
> Ideally for metro the main content thread would still be called 'GeckoMain'
> since that threads is treated differently. But right now the profiler
> assumes that the thread that calls profiler_init is the GeckoMain thread so
> we'd have to break this assumption. This is only important so higher level
> features like bug 918825 will work on metro, we could punt on that.

Sounds like we'll need:

1) the profiler will need a fix to break the relationship between 'GeckoMain' and the thread that calls profiler_init. 
2) the real gecko thread in metro will need to register as 'GeckoMain'.
3) Looks like the compositor thread also needs to be registered.

On 1, maybe for metro we can simply ifdef this bit of code to use a different name - 
http://mxr.mozilla.org/mozilla-central/source/tools/profiler/platform.cpp#392

Comment 2

[Benoit Girard (:BenWa)]

Does metro ship a different XUL binary then desktop firefox? If not we can't pre-process ifdef there.

Comment 3

[(no longer active)]

(In reply to comment #2)
> Does metro ship a different XUL binary then desktop firefox?

No.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Does metro ship a different XUL binary then desktop firefox? If not we can't
> pre-process ifdef there.

Ah yes, sorry, sometimes I forget that we share everything. I bet we can use XRE_GetWindowsEnvironment() instead.

Comment 5

[Jim Mathies [:jimm]]

Attachment: patch

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 819101 [details] [diff] [review]
patch

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

I'd like for GeckoMain to always be considered the main thread even on Metro otherwise we're going to see more problems.

::: tools/profiler/platform.cpp
@@ +392,5 @@
>  
> +#ifdef XP_WIN
> +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) {
> +    // We'll register GeckoMain once its gets created down in winrt widget.
> +    Sampler::RegisterCurrentThread("Application Thread", stack, true, stackTop);

You're still setting this to be the main thread. true/false and perhaps modify the register thread to have GeckoMain imply that it's the main thread?

@@ +394,5 @@
> +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) {
> +    // We'll register GeckoMain once its gets created down in winrt widget.
> +    Sampler::RegisterCurrentThread("Application Thread", stack, true, stackTop);
> +  } else {
> +    Sampler::RegisterCurrentThread("GeckoMain", stack, true, stackTop);

This should be shared with the non ifdef path

::: widget/windows/winrt/MetroApp.cpp
@@ +112,5 @@
>    // Shut down xpcom
>    XRE_metroShutdown();
> +
> +  // Unhook this thread from the profiler
> +  profiler_unregister_thread();

LGTM as long as there's no early return in between the context here.

Comment 7

[Jim Mathies [:jimm]]

> > +#ifdef XP_WIN
> > +  if (XRE_GetWindowsEnvironment() == WindowsEnvironmentType_Metro) {
> > +    // We'll register GeckoMain once its gets created down in winrt widget.
> > +    Sampler::RegisterCurrentThread("Application Thread", stack, true, stackTop);
> 
> You're still setting this to be the main thread. true/false and perhaps
> modify the register thread to have GeckoMain imply that it's the main thread?

Ah I see, I was thinking 'GeckoMain' was the token that identified the main thread. I see I need to pass a param to the sampler register function.

Comment 8

[Benoit Girard (:BenWa)]

Honestly you could just remove the bool from the signature and infer it based on the name. That would probably be better.

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 819898 [details] [diff] [review]
patch v.2

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

Drive-by review. LGTM.

::: tools/profiler/platform.cpp
@@ +378,5 @@
> +bool is_main_thread_name(const char* aName) {
> +  if (aName) {
> +    return false;
> +  }
> +  return !strcmp(aName, gGeckoThreadName);

nit: I don't like how this reads 'if it does not string compare' feels like it implies the opposite, I rather == 0.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Benoit Girard (:BenWa) from comment #10)
> Comment on attachment 819898 [details] [diff] [review]
> patch v.2
> 
> Review of attachment 819898 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by review. LGTM.
> 
> ::: tools/profiler/platform.cpp
> @@ +378,5 @@
> > +bool is_main_thread_name(const char* aName) {
> > +  if (aName) {
> > +    return false;
> > +  }
> > +  return !strcmp(aName, gGeckoThreadName);
> 
> nit: I don't like how this reads 'if it does not string compare' feels like
> it implies the opposite, I rather == 0.

I debated using that form when I wrote it, mozilla's coding rules won out but I didn't like it either. I'm fine with this change.

Thanks for the review.

Comment 12

[Benoit Girard (:BenWa)]

I use it for pointers but I try avoiding it when I expect the value 0 but use the proper style for pointer cehcks. I know it's a bit fuzzy =\. I override the style guide if readability is effected.

Comment 13

[Jim Mathies [:jimm]]

try run:
https://tbpl.mozilla.org/?tree=Try&rev=b177477ba3f8

https://hg.mozilla.org/integration/fx-team/rev/bb65076aabd8

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bb65076aabd8

Comment 15

[Jim Mathies [:jimm]]

Attachment: fix

Bah! Simple patch to fix a silly mistake.

Comment 16

[Jim Mathies [:jimm]]

One additional problem I'm having here. When we register the main thread late like we do in metro, you don't get other threads in the profile. I've checked and they are registering, but the profile doesn't seem to pick them up. The only thread I see is GeckoMain.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Jim Mathies [:jimm] from comment #16)
> One additional problem I'm having here. When we register the main thread
> late like we do in metro, you don't get other threads in the profile. I've
> checked and they are registering, but the profile doesn't seem to pick them
> up. The only thread I see is GeckoMain.

You want to make sure that in Sampler::RegisterCurrentThread they end up in sRegisteredThreads. This will also call TableTicker::RegisterThread which will check if we have this thread currently selected an allocate a ThreadProfile for it. Then in the sampling inside Thread::Run they should be read from sRegisteredThreads.

If you don't have 'profiler.threads' set to true then of course only the main thread will be profiled.

Comment 18

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/1d65a43d4ed0

Thanks, will get this going again today and check over all the prefs.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d65a43d4ed0