1224810 - TraceLogger: Add the script information for the event created by BytecodeCompiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wei Wu [:wuwei UTC+8]]

Peeled from bug 1223636.
see bug 1223636 comment #7 for more details.

Comment 1

[Wei Wu [:wuwei UTC+8]]

Attachment: bug1224810.patch

the previous version is attachment 8685999 [details] [diff] [review].
fixed style-nit (bug 1223636 comment 5).

asking r?h4writer when we come to an agreement on bug 1223636 comment 6.

Comment 2

[Wei Wu [:wuwei UTC+8]]

Comment on attachment 8687546 [details] [diff] [review]
bug1224810.patch

talked to h4writer. we're agree on this bug. r?h4writer.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 8687546 [details] [diff] [review]
bug1224810.patch

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

Looks good. Two issues with logging non-"script events" with 'script information'

::: js/src/frontend/BytecodeCompiler.cpp
@@ +132,5 @@
>                                     const ReadOnlyCompileOptions& options,
>                                     SourceBufferHolder& sourceBuffer,
>                                     Handle<ScopeObject*> enclosingStaticScope,
>                                     TraceLoggerTextId logId)
> +  : traceLogger(cx, logId, options),

This should be 'traceLogger(cx, logId)', since the given logId is not a "script event"
Script events being currently: TraceLogger_Scripts, TraceLogger_AnnotateScripts and TraceLogger_InlinedScripts
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TraceLogging.cpp#408

@@ +782,5 @@
>             .setColumn(lazy->column())
>             .setNoScriptRval(false)
>             .setSelfHostingMode(false);
>  
> +    AutoCompilationTraceLogger traceLogger(cx, TraceLogger_ParserCompileLazy, options);

This should be 'AutoCompilationTraceLogger traceLogger(cx, TraceLogger_ParserCompileLazy)', since the TraceLogger_ParserCompileLazy is not a "script event"
Script events being currently: TraceLogger_Scripts, TraceLogger_AnnotateScripts and TraceLogger_InlinedScripts
https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TraceLogging.cpp#408

Can you create a constructor for AutoCompilationTraceLogger that takes no script information (options) to facilitate this?

Comment 4

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #3)
> Comment on attachment 8687546 [details] [diff] [review]
> bug1224810.patch
> 
> Review of attachment 8687546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good. Two issues with logging non-"script events" with 'script
> information'
> 
> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +132,5 @@
> >                                     const ReadOnlyCompileOptions& options,
> >                                     SourceBufferHolder& sourceBuffer,
> >                                     Handle<ScopeObject*> enclosingStaticScope,
> >                                     TraceLoggerTextId logId)
> > +  : traceLogger(cx, logId, options),
> 
> This should be 'traceLogger(cx, logId)', since the given logId is not a
> "script event"
> Script events being currently: TraceLogger_Scripts,
> TraceLogger_AnnotateScripts and TraceLogger_InlinedScripts
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TraceLogging.cpp#408
> 
> @@ +782,5 @@
> >             .setColumn(lazy->column())
> >             .setNoScriptRval(false)
> >             .setSelfHostingMode(false);
> >  
> > +    AutoCompilationTraceLogger traceLogger(cx, TraceLogger_ParserCompileLazy, options);
> 
> This should be 'AutoCompilationTraceLogger traceLogger(cx,
> TraceLogger_ParserCompileLazy)', since the TraceLogger_ParserCompileLazy is
> not a "script event"
> Script events being currently: TraceLogger_Scripts,
> TraceLogger_AnnotateScripts and TraceLogger_InlinedScripts
> https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TraceLogging.cpp#408
> 
> Can you create a constructor for AutoCompilationTraceLogger that takes no
> script information (options) to facilitate this?

AutoCompilationTraceLogger creates a TraceLogger_Scripts implicitly, which consumes ReadOnlyCompileOptions. Unfortunately I failed to find another way for creating scripts events w/o passing the options that needed :(

I however have two workarounds:
1. Don't use AutoCompilationTraceLogger. Create scripts event and logger for ParserCompileXXX explicitly. This actually "reverts" the original commit;
2. Create three different AutoCompilationTraceLogger for each ParserCompileXXX events. Sounds not good.

I think I need some more advice here :)

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8687546 [details] [diff] [review]
bug1224810.patch

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

Ok, my bad. This is totally correct!

Comment 6

[Wei Wu [:wuwei UTC+8]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> Comment on attachment 8687546 [details] [diff] [review]
> bug1224810.patch
> 
> Review of attachment 8687546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, my bad. This is totally correct!

np, thanks :)

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f37799705530

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/f37799705530