Closed Bug 996509 Opened 10 years ago Closed 9 years ago

Tracelogger: Make it possible to toggle when scripts get logged.

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(2 files, 1 obsolete file)

I think this is the last step before we can ship tracelogger by default!
Depends on: 944392
Blocks: 1008846
Blocks: 1065722
Last piece. Now we can build tracelogger without performance penalty when not active :D

This creates a "Scripts" logger, which enables to enable/disable logging of scripts. (All other items where already toggle-able).
Assignee: nobody → hv1989
Attachment #8494535 - Flags: review?(benj)
This is just to make sure people don't disable a specific engine from logging, which would totally corrupt the tree.

As a result "stopEvent" without a textId can now be private. Which means it is harder to do wrong things ;).
Attachment #8495861 - Flags: review?(benj)
Noticed a few issues during testing of next patch. This should address them.
Attachment #8495861 - Attachment is obsolete: true
Attachment #8495861 - Flags: review?(benj)
Attachment #8495906 - Flags: review?(benj)
Comment on attachment 8494535 [details] [diff] [review]
Tl-toggle-script-logging

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

Looks good to me, sorry for the delay.

::: js/src/jit/IonMacroAssembler.cpp
@@ -1450,5 @@
>      passABIArg(logger);
>      passABIArg(textId);
>      callWithABINoProfiling(JS_FUNC_TO_DATA_PTR(void *, TraceLogFunc));
>  
> -    regs.add(temp);

good catch

@@ +1518,1 @@
>      callWithABINoProfiling(JS_FUNC_TO_DATA_PTR(void *, TraceLogFunc));

While you're here, can you have the callWithABINoProfiling be inside the #if DEBUG / #else sections? That would spare declarations of TraceLogFunc which are, ahem, not pleasant.

@@ +1522,5 @@
>  
> +void
> +MacroAssembler::tracelogStopScript(Register logger)
> +{
> +    tracelogStop(logger, TraceLogger::Scripts);

Could you instead manually inline this function?

::: js/src/vm/TraceLogging.cpp
@@ +377,5 @@
>          return createTextId("");
>  
>      assertNoQuotes(script->filename());
>  
> +    // Only log scripts when enabled. Else return the global Scripts textId,

s/Else/Otherwise

@@ +378,5 @@
>  
>      assertNoQuotes(script->filename());
>  
> +    // Only log scripts when enabled. Else return the global Scripts textId,
> +    // which will get filtered out.

That happens because the Scripts textId is disabled by default, right?

::: js/src/vm/TraceLogging.h
@@ +126,5 @@
>      _(TL)                                             \
>      _(IrregexpCompile)                                \
>      _(IrregexpExecute)                                \
>      _(VM)                                             \
> +    _(Scripts)                                        \

Seems there was some alphabetical order before Irregexp{Compile,Execute}, feel free to enforce it again while you're here.
Attachment #8494535 - Flags: review?(benj) → review+
Comment on attachment 8495906 [details] [diff] [review]
Add "engine" logging entry and disable toggling specific engines

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

Nice!

::: js/src/jit/IonMacroAssembler.cpp
@@ +1473,1 @@
>      void (&TraceLogFunc)(TraceLogger*, uint32_t) = TraceLogStopEvent;

Now inlining the use appears obvious :)

@@ +1492,1 @@
>      void (&TraceLogFunc)(TraceLogger*, uint32_t) = TraceLogStopEvent;

Same here.

::: js/src/vm/TraceLogging.cpp
@@ +681,5 @@
>  TraceLogger::stopEvent(uint32_t id)
>  {
>  #ifdef DEBUG
> +    if (id != TraceLogger::Scripts && id != TraceLogger::Engine &&
> +        stack.size() > 1 && stack.lastEntry().active())

Just for readability purposes, could you please give each condition its own line?

@@ +815,5 @@
> +    for (uint32_t i = 1; i < TraceLogger::LAST; i++) {
> +        if (TraceLogger::textIdIsToggable(i))
> +            enabledTextIds[i] = ContainsFlag(env, text[i]);
> +        else
> +            enabledTextIds[i] = true;

maybe pedantic, but you could have

enabledTextIds[i] = !Tracelogger::textIdIsToggable(i) || ContainsFlag(env, text[i]);

::: js/src/vm/TraceLogging.h
@@ +434,5 @@
>  
> +    static bool textIdIsToggable(uint32_t id) {
> +        if (id == TL_Error)
> +            return false;
> +        if (id == TL)

Can you group these two?
Attachment #8495906 - Flags: review?(benj) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> @@ +378,5 @@
> >  
> >      assertNoQuotes(script->filename());
> >  
> > +    // Only log scripts when enabled. Else return the global Scripts textId,
> > +    // which will get filtered out.
> 
> That happens because the Scripts textId is disabled by default, right?

Yes (I think you understand).

But I'll rephrase to be certain:
If logging a script is enabled, we will log the specific TextId. (Which we cannot distinguish as being enabled or disabled). If disabled we will log TraceLogger::Scripts. In startEvent we will be able to detect TraceLogger::Scripts is disabled and not log that event.
Hi, sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-central&revision=b9fd2074d588 since with the landings of this changesets we had permafailures in ggc tests like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=661496&repo=mozilla-central
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/3ccf3fac28b7
https://hg.mozilla.org/mozilla-central/rev/648947a5a445
Status: REOPENED → RESOLVED
Closed: 10 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla36 → mozilla37
You need to log in before you can comment on or make changes to this bug.