Closed Bug 1091479 Opened 10 years ago Closed 9 years ago

TaskTracer: SourceEvent have no record of vptr

Categories

(Core :: Gecko Profiler, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: gyeh, Assigned: gyeh)

References

Details

Attachments

(2 files, 4 obsolete files)

From the logging results, we found that all SourceEvents have no record of vptr which results in no symbol can be successfully extracted from libxul.so
No longer depends on: 1089514, 1091533
Assignee: nobody → gyeh
Comment on attachment 8518794 [details] [diff] [review]
Patch 1(v1): Log dispatch time when creating a source event

Dispatch time is missing in source event.
Attachment #8518794 - Flags: review?(tlee)
Comment on attachment 8518794 [details] [diff] [review]
Patch 1(v1): Log dispatch time when creating a source event

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

::: tools/profiler/GeckoTaskTracer.cpp
@@ +99,5 @@
>    info->mCurTaskId = newId;
>  
>    // Log a fake dispatch and start for this source event.
> +  LogDispatch(newId, newId, newId, aType);
> +  LogVirtualTablePtr(newId, newId, (int*)gFuncPtr);

I think, maybe we could use a more meaningful way.  For example, every type has a symbol.

int Type1Event;
int Type2Event;
int Type3Event;

static void *sEventSymbols[] = { &Type1Event, &Type2Event, &Type3Event };

CreateSourceEvent(SourceEventType aType)
{
   ...
   MOZ_ASSERT(aType < (sizeof(sEventSymbols) / sizeof(sEventSymbols[0])));
   LogVirtualTablePtr(newId, newId, sEventSymbols[aType]);
   ...
}

We could define all event types in a included file with a macro.  The file is used to generate SourceEventType and symbols.  How do you think?
Sounds great to me. :)
- Create "SourceEventTypeMap.h"
- Add macro GET_SOURCE_EVENT_NAME
Attachment #8518794 - Attachment is obsolete: true
Attachment #8518794 - Flags: review?(tlee)
Comment on attachment 8525919 [details] [diff] [review]
Patch 1(v2): Log dispatch time when creating a source event

Comments are addressed. Please review the new patch.
Attachment #8525919 - Flags: review?(tlee)
The macro is refined.
Attachment #8525919 - Attachment is obsolete: true
Attachment #8525919 - Flags: review?(tlee)
Attached patch Patch 2(v1): Rename enum members (obsolete) — Splinter Review
Attachment #8526574 - Flags: review?(tlee)
Attachment #8526573 - Flags: review?(tlee)
Comment on attachment 8526573 [details] [diff] [review]
Patch 1(v3): Log dispatch time when creating a source event

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

::: tools/profiler/GeckoTaskTracer.cpp
@@ +97,5 @@
>  
> +  int* namePtr;
> +#define SOURCE_EVENT_NAME(type)         \
> +  case SourceEventType::type:           \
> +    static int CreateSourceEvent##type; \

This may cause compiler warning.  Please embrace it with brackets.
Attachment #8526573 - Flags: review?(tlee) → review+
Attachment #8526574 - Flags: review?(tlee) → review+
(In reply to Thinker Li [:sinker] from comment #9)
> Comment on attachment 8526573 [details] [diff] [review]
> Patch 1(v3): Log dispatch time when creating a source event
> 
> Review of attachment 8526573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tools/profiler/GeckoTaskTracer.cpp
> @@ +97,5 @@
> >  
> > +  int* namePtr;
> > +#define SOURCE_EVENT_NAME(type)         \
> > +  case SourceEventType::type:           \
> > +    static int CreateSourceEvent##type; \
> 
> This may cause compiler warning.  Please embrace it with brackets.

Thanks. I'll update my final patch.
Attachment #8526574 - Attachment is obsolete: true
Attachment #8526625 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/937387b0d0bf
https://hg.mozilla.org/mozilla-central/rev/c178550109f8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1104414
Blocks: 1111949
Blocks: 1111952
No longer blocks: 1111952
You need to log in before you can comment on or make changes to this bug.