1167634 - Make TaskTracer build and go on Windows

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Comment 1

[Honza Bambas (:mayhemer)]

bugzilla is broken, cannot attach a patch...

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 3

[Shelly Lin [:shelly]]

Comment on attachment 8609872 [details] [diff] [review]
v1

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

Thanks for looking up TaskTracer on Windows, I've left some comments below, but Thinker might be a better fit for reviewer :)

::: tools/profiler/GeckoTaskTracer.cpp
@@ +217,5 @@
>  
>  void
>  InitTaskTracer(uint32_t aFlags)
>  {
> +  if (sInitCount++) {

Does it get double init on Win platform?
This will also break TaskTracer on b2g.

::: tools/profiler/TracedTaskCommon.h
@@ +74,5 @@
>  // their parents, e.g. The timer events.
>  class FakeTracedTask : public TracedTaskCommon
>  {
>  public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FakeTracedTask)

Issue of FakeTracedTask will be fixed in bug 1113562, and the nit fix of TracedTaskCommon as well.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Shelly Lin [:shelly] from comment #3)
> Comment on attachment 8609872 [details] [diff] [review]
> v1
> 
> Review of attachment 8609872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for looking up TaskTracer on Windows, I've left some comments below,
> but Thinker might be a better fit for reviewer :)
> 
> ::: tools/profiler/GeckoTaskTracer.cpp
> @@ +217,5 @@
> >  
> >  void
> >  InitTaskTracer(uint32_t aFlags)
> >  {
> > +  if (sInitCount++) {
> 
> Does it get double init on Win platform?

Yes, I can give you callstacks if you want.

> This will also break TaskTracer on b2g.

Are you sure?  If the system is initialized just once, this change should have no effect at all.

> 
> ::: tools/profiler/TracedTaskCommon.h
> @@ +74,5 @@
> >  // their parents, e.g. The timer events.
> >  class FakeTracedTask : public TracedTaskCommon
> >  {
> >  public:
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(FakeTracedTask)
> 
> Issue of FakeTracedTask will be fixed in bug 1113562, and the nit fix of
> TracedTaskCommon as well.

Good!

Comment 5

[Thinker Li [:sinker]]

Comment on attachment 8609872 [details] [diff] [review]
v1

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

Sorry for late!  This patch is quite close to r+, but there is some minor changes or explanations required.

::: tools/profiler/GeckoTaskTracer.cpp
@@ +285,5 @@
>  {
>    TraceInfo* info = GetOrCreateTraceInfo();
>    ENSURE_TRUE(info, 0);
>  
> +  uint32_t tid = gettid();

Please define pid_t as uint32_t, instead of changing code here.

::: tools/profiler/TracedTaskCommon.cpp
@@ +21,5 @@
>  TracedTaskCommon::Init()
>  {
>    TraceInfo* info = GetOrCreateTraceInfo();
> +  if (!info)
> +    return;

Why do we need to change this line?  I expect info is always not null.  Could you explain it

::: xpcom/base/VisualEventTracer.cpp
@@ +628,5 @@
>  
>    int32_t bytesWritten = PR_Write(fd, json.get(), json.Length());
>    PR_Close(fd);
>  
> +  if (bytesWritten < 0 || (uint32_t)bytesWritten < json.Length()) {

I guess |bytesWritten < json.length()| is always true if |bytesWritten < 0|.  So, we can remove |bytesWritten < 0| here.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Thinker Li [:sinker] from comment #5)
> Comment on attachment 8609872 [details] [diff] [review]
> v1
> 
> Review of attachment 8609872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for late!  This patch is quite close to r+, but there is some minor
> changes or explanations required.
> 
> ::: tools/profiler/GeckoTaskTracer.cpp
> @@ +285,5 @@
> >  {
> >    TraceInfo* info = GetOrCreateTraceInfo();
> >    ENSURE_TRUE(info, 0);
> >  
> > +  uint32_t tid = gettid();
> 
> Please define pid_t as uint32_t, instead of changing code here.
> 
> ::: tools/profiler/TracedTaskCommon.cpp
> @@ +21,5 @@
> >  TracedTaskCommon::Init()
> >  {
> >    TraceInfo* info = GetOrCreateTraceInfo();
> > +  if (!info)
> > +    return;
> 
> Why do we need to change this line?  I expect info is always not null. 
> Could you explain it

This is probably a left over from times GetOrCreateTraceInfo() didn't work on Win.  I'll check whether it's fixed or not (and add an explanatory comment).

> 
> ::: xpcom/base/VisualEventTracer.cpp
> @@ +628,5 @@
> >  
> >    int32_t bytesWritten = PR_Write(fd, json.get(), json.Length());
> >    PR_Close(fd);
> >  
> > +  if (bytesWritten < 0 || (uint32_t)bytesWritten < json.Length()) {
> 
> I guess |bytesWritten < json.length()| is always true if |bytesWritten < 0|.
> So, we can remove |bytesWritten < 0| here.

This code goes away completely in bug 1170534, so I can remove this change (I'll have to remerge anyway).

Comment 7

[Honza Bambas (:mayhemer)]

According my recent knowledge, TT should now build and run on desktop (mainly on Win) right now.  Can you please confirm it?  If so, I can close this bug then.

Comment 8

[Thinker Li [:sinker]]

Cervantes is the right person to confirm it now.

Comment 9

[Cervantes Yu [:cyu] [:cervantes]]

Bug 1221846 fixes TT on Linux and MacOS. I am not sure about Windows, but as for gettid() implementation, I don't think it works on Windows. At least we'll need gettid() counterpart on Windows in this patch. So I suggest we leave this bug open and fix what's missing on Windows.

Comment 10

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: The minimal patch to fix TT breakage on Windows

This fixes the missing gettid() on Windows.

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #10)
> Created attachment 8708288 [details] [diff] [review]
> The minimal patch to fix TT breakage on Windows
> 
> This fixes the missing gettid() on Windows.

Doesn't work.  Anyway, I no longer want to be dependent on Task Tracer.  I can see many more places I need to hook anyway.

Comment 12

[Markus Stange [:mstange]]

Is this fixed?

Comment 13

[Thinker Li [:sinker]]

Cervantes know this better.

Comment 14

[Cervantes Yu [:cyu] [:cervantes]]

TaskTracer builds on latest m-c now. I will check if it functions correctly.

Comment 15

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: LogVirtualTablePtr() on Windows

Comment 16

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: nsTimerEvent vtable for nsIRunnable

Comment 17

[Cervantes Yu [:cyu] [:cervantes]]

I think LogVirtualTablePtr() works correctly on Windows. See the screenshots from the VS debug session:

When LogVirtualTablePtr() is called, parameter aVptr contains the value 0x7ff87e3d0aa8. VS recognizes it as {xul.dll!const nsTimerEvent::`vtable`for{`nsIRunnable`}} (attachment 8821494 [details]). To verify that the value is correct, go up several frames to where the event is dispatched (attachment 8821495 [details]), it can be seen that the runnable is actually an instance of nsTimerEvent, and the vptr value matches what is recorded in LogVirtualTablePtr().

So what we need to do is symbolication of the logged vptr values with the PDB symbols.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Is there any action needed for this bug?

Comment 19

[Cervantes Yu [:cyu] [:cervantes]]

On the latest m-c and perf-html.io, there is nothing to do for this bug. I think we can close it.