1221844 - TraceLogger: Move 'TraceLogger_Invalidation' to "LOG_ITEM" (used to be "TREE_ITEM").

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wei Wu [:wuwei UTC+8]]

Attachment: tl-mandreel-invalidation-bug

'TraceLogger_Invalidation' is belong to 'TRACELOGGER_TREE_ITEMS'. It would be better to use "AutoTraceLog" instead of timestamp to log 'TraceLogger_Invalidation', otherwise the tracelogger might complain (see attachment).

Comment 1

[Wei Wu [:wuwei UTC+8]]

Attachment: patch

one line patch.

Comment 2

[Hannes Verschore [:h4writer]]

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

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

The 'TRACELOGGER_TREE_ITEMS' are for items where we would like to track the time it took.
For Invalidation/Bailouts that is not something we want to track. In that case it is more important to know how many times it has occurred.

Your patch indeed solves the issue, by treating Invalidation like a tree_item. Though I would prefer to go the other way and
make Invalidation similar to Bailouts. Can you update and move Invalidation to the 'TRACELOGGER_LOG_ITEMS' list?

Bonus points if you add some debug checks to make sure the correct type is used for the logTimestamp/start/stop functions.

Comment 3

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #2)
> Comment on attachment 8683427 [details] [diff] [review]
> patch
> 
> Review of attachment 8683427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The 'TRACELOGGER_TREE_ITEMS' are for items where we would like to track the
> time it took.
> For Invalidation/Bailouts that is not something we want to track. In that
> case it is more important to know how many times it has occurred.
> 
> Your patch indeed solves the issue, by treating Invalidation like a
> tree_item. Though I would prefer to go the other way and
> make Invalidation similar to Bailouts. Can you update and move Invalidation
> to the 'TRACELOGGER_LOG_ITEMS' list?

Sure. :)


> Bonus points if you add some debug checks to make sure the correct type is
> used for the logTimestamp/start/stop functions.

IIUC these checks would be similar with the checks in TLTextIdIsTreeEvent().

Comment 4

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1221844.patch

move to LOG_ITEMS.

It works.

Comment 5

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1221844b.patch

This patch added range checks in TraceLogTimestamp().

For TREE_ITEMS events, There is a function named 'TLTextIdIsTreeEvent(uint32_t)' guarding for them.

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 8686534 [details] [diff] [review]
bug1221844.patch

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

Thanks!

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 8686537 [details] [diff] [review]
bug1221844b.patch

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

::: js/src/vm/TraceLogging.h
@@ +389,4 @@
>  #endif
>  inline void TraceLogTimestamp(TraceLoggerThread* logger, TraceLoggerTextId textId) {
>  #ifdef JS_TRACE_LOGGING
> +    MOZ_ASSERT(textId > TraceLogger_LastTreeItem && textId < TraceLogger_Last);

Can you add this in TraceLogger::logTimestamp?

Comment 8

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #7)
> Comment on attachment 8686537 [details] [diff] [review]
> bug1221844b.patch
> 
> Review of attachment 8686537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/TraceLogging.h
> @@ +389,4 @@
> >  #endif
> >  inline void TraceLogTimestamp(TraceLoggerThread* logger, TraceLoggerTextId textId) {
> >  #ifdef JS_TRACE_LOGGING
> > +    MOZ_ASSERT(textId > TraceLogger_LastTreeItem && textId < TraceLogger_Last);
> 
> Can you add this in TraceLogger::logTimestamp?

I'm afraid not.
Currently the start/stopEvent() are just wrappers for logTimestamp(), adding checks in logTimestamp() will crash start/stopEvent().

TraceLogger::logTimestamp(uint32_t) was renamed to
TraceLoggerThread::logTimestamp(uint32_t), which is used by
TraceLoggerThread::startEvent(uint32_t id), which accepts TREE_ITEM events.

https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TraceLogging.cpp#498

Comment 9

[Hannes Verschore [:h4writer]]

(In reply to Wei Wu [:w :wuwei UTC+8] from comment #8)
> (In reply to Hannes Verschore [:h4writer] from comment #7)
> > Comment on attachment 8686537 [details] [diff] [review]
> > bug1221844b.patch
> > 
> > Review of attachment 8686537 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/vm/TraceLogging.h
> > @@ +389,4 @@
> > >  #endif
> > >  inline void TraceLogTimestamp(TraceLoggerThread* logger, TraceLoggerTextId textId) {
> > >  #ifdef JS_TRACE_LOGGING
> > > +    MOZ_ASSERT(textId > TraceLogger_LastTreeItem && textId < TraceLogger_Last);
> > 
> > Can you add this in TraceLogger::logTimestamp?
> 
> I'm afraid not.
> Currently the start/stopEvent() are just wrappers for logTimestamp(), adding
> checks in logTimestamp() will crash start/stopEvent().
> 
> TraceLogger::logTimestamp(uint32_t) was renamed to
> TraceLoggerThread::logTimestamp(uint32_t), which is used by
> TraceLoggerThread::startEvent(uint32_t id), which accepts TREE_ITEM events.
> 
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TraceLogging.cpp#498

oh right. Correct.
What about creating a protected function "log", which does the internals of logTimestamp and have logTimestamp and startEvent/stopEvent call this protected function "log"?. In that case we can add the checks in logTimestamp...

Comment 10

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1221844b.patch

(In reply to Hannes Verschore [:h4writer] from comment #9)
> oh right. Correct.
> What about creating a protected function "log", which does the internals of
> logTimestamp and have logTimestamp and startEvent/stopEvent call this
> protected function "log"?. In that case we can add the checks in
> logTimestamp...

updated version. passed octane profile generation.
not sure whether Debugger would be affected, ni? instead of r?

so is this patch ok?

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 8688797 [details] [diff] [review]
bug1221844b.patch

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

Yes indeed. Looks good.

Comment 12

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 1: TraceLogger: Move TraceLogger_Invalidation to LOG_ITEM.

rebased.

Comment 13

[Wei Wu [:wuwei UTC+8]]

Attachment: Part 2: TraceLogger: Add some debug checks to logTimestamp.

rebased.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abb793676e19
https://hg.mozilla.org/integration/mozilla-inbound/rev/c06b198b8167

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/abb793676e19
https://hg.mozilla.org/mozilla-central/rev/c06b198b8167