Closed Bug 1167634 Opened 5 years ago Closed 3 years ago

Make TaskTracer build and go on Windows

Categories

(Core :: Gecko Profiler, defect)

All
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox41 --- affected

People

(Reporter: mayhemer, Unassigned)

References

Details

Attachments

(4 files)

No description provided.
bugzilla is broken, cannot attach a patch...
Attached patch v1Splinter Review
Attachment #8609872 - Flags: review?(slin)
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.
Attachment #8609872 - Flags: review?(slin) → review?(tlee)
(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!
Depends on: 1113562
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.
Attachment #8609872 - Flags: review?(tlee) → review-
(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).
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.
Flags: needinfo?(tlee)
Cervantes is the right person to confirm it now.
Flags: needinfo?(tlee) → needinfo?(cyu)
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.
Flags: needinfo?(cyu)
See Also: → 1221846
This fixes the missing gettid() on Windows.
(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.
Assignee: honzab.moz → nobody
No longer blocks: 1148204
Status: ASSIGNED → NEW
Is this fixed?
Flags: needinfo?(tlee)
Cervantes know this better.
Flags: needinfo?(tlee) → needinfo?(cyu)
TaskTracer builds on latest m-c now. I will check if it functions correctly.
Flags: needinfo?(cyu)
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.
Is there any action needed for this bug?
Blocks: 1323076
Flags: needinfo?(cyu)
On the latest m-c and perf-html.io, there is nothing to do for this bug. I think we can close it.
Flags: needinfo?(cyu)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.