Closed Bug 927979 Opened 6 years ago Closed 6 years ago

Register the gecko thread correctly in the profiler for FxMetro

Categories

(Core :: Gecko Profiler, defect)

x86
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: kats, Assigned: jimm)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
(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
Does metro ship a different XUL binary then desktop firefox? If not we can't pre-process ifdef there.
(In reply to comment #2)
> Does metro ship a different XUL binary then desktop firefox?

No.
(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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → jmathies
Attachment #819101 - Flags: review?(bgirard)
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.
Attachment #819101 - Flags: review?(bgirard) → review-
> > +#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.
Honestly you could just remove the bool from the signature and infer it based on the name. That would probably be better.
Attached patch patch v.2Splinter Review
Attachment #819101 - Attachment is obsolete: true
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.
Attachment #819898 - Flags: review+
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/bb65076aabd8
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Attached patch fixSplinter Review
Bah! Simple patch to fix a silly mistake.
Attachment #825465 - Flags: review?
Attachment #825465 - Flags: review? → review?(bgirard)
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.
Attachment #825465 - Flags: review?(bgirard) → review+
(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.
https://hg.mozilla.org/integration/fx-team/rev/1d65a43d4ed0

Thanks, will get this going again today and check over all the prefs.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.