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)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: h4writer, Assigned: h4writer)
References
Details
Attachments
(2 files, 1 obsolete file)
15.51 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
10.38 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
I think this is the last step before we can ship tracelogger by default!
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
(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.
Assignee | ||
Comment 7•10 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 9•10 years ago
|
||
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 ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla36 → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•