I think this is the last step before we can ship tracelogger by default!

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).

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 ;).

Noticed a few issues during testing of next patch. This should address them.

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.

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?

(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.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c168f572252 https://hg.mozilla.org/integration/mozilla-inbound/rev/2b29d8a00774

https://hg.mozilla.org/mozilla-central/rev/2b29d8a00774 https://hg.mozilla.org/mozilla-central/rev/5c168f572252

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

https://hg.mozilla.org/mozilla-central/rev/3ccf3fac28b7 https://hg.mozilla.org/mozilla-central/rev/648947a5a445