In tracelogging 0.1 logging happened in a list. In 0.2 this was changed to a tree-like structure. This had some nice properties since it is more restrictive on wrong emitting wrong sequence of items. Also it was easier to debug ... Nexto this we also had a list with non-tree log items. (e.g. an bailout happened). The tree was used a lot and the log list was not used at all. This had some drawbacks. The log code itself is larger than expected and as a result harder to inline (in asm). Also it would be better if the Debugger can use a normal log list. The idea is use the 'log list' to log. And have an additional class/function to create the graph out of this log. This has the benefit of: - Generating asm code would be easier again (speed++) - Generation of the graph can be done off-thread (no need for TL to pause while saving data to file when memory was full). - Code will be simpler - Easier for Debugger to tap in. - log events will not be second citizen anymore. - Make it easier to toggle "graph creation"/"Debugger hooks" off/on So for maintainability and extensibility I think this is the path forward. This is the minimal changes I think are needed in order to get the "Debugger hooks" in a bit sanely. In my plan I have some more changes planned which will make it even better, but this is a bit a stop-gap. Functionality-wise not a lot will change. It will be mostly moving/refacturing.

Create a function to retrieve the text corresponding to a TextId

The next part will split some files and the TextIds should be available in both files. So ripping the enum out of TraceLogger.

> js/src/moz.build | 1 + > js/src/vm/TraceLogging.cpp | 692 +++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------------------------------------- > js/src/vm/TraceLogging.h | 373 +++------------------------------------------------------------------------------------- > js/src/vm/TraceLoggingGraph.cpp | 513 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > js/src/vm/TraceLoggingGraph.h | 245 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > js/src/vm/TraceLoggingTypes.h | 269 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 6 files changed, 1176 insertions(+), 917 deletions(-) The intention is to be able to toggle the graph creation independently of logging. This file splits TraceLogger into TraceLogger and TraceLoggerGraph. I made sure this is as minimal as possible. So almost nothing changes except for splitting these files and the bare minimum to tie everything together again. The next patches will be much smaller again, making incremental changes.

Comment on attachment 8498493 [details] [diff] [review] Part 3: Split graph creation out of logging. Review of attachment 8498493 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TraceLogging.cpp @@ +19,1 @@ > #include "vm/Runtime.h" My alphabet is weird :S

Comment on attachment 8498767 [details] [diff] [review] Part 4: Free the things! Review of attachment 8498767 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TraceLoggingTypes.h @@ +183,5 @@ > > + ~ContinuousSpace() > + { > + js_free(data_); > + data_ == nullptr; oops s/==/=/

In the grand picture we want to remove all graph.XXX calls from TraceLogger. Since calling graph.log(events) means we need to clear all logged event. This is bad if we have two consumers (graph & debugger). So removing the graph.log() when enabling/disabling a logger is a step forward towards this. (This brings us also closer to TraceLoggerGraph working on a background thread, which I want to accomplish after having the Debugger hooks! Decreasing the influence of tracelogger even more \0/)

TraceLogging and TraceLoggingGraph are called from different threads. TraceLogging already locks for public functions. This neeeds to get done for TraceLoggingGraph too. (This is the difference between TraceLogger and TraceLogging. TraceLogging is a collection of loggers on different threads. So TraceLogger doesn't need to care about threads, TraceLogging does.

Comment on attachment 8498767 [details] [diff] [review] Part 4: Free the things! Review of attachment 8498767 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TraceLogging.cpp @@ +72,5 @@ > { > if (!failed) > graph.log(events); > + > + for (uint32_t i = 0; extraText.length(); i++) { i < extraText.length()

Final cornerstone of this bug. Make it possible to toggle graph creation. As a result the tracelogger graphs will still work, but not required to let Debugger hooks work. TLOPTIONS=EnableGraph is now needed to enable graph creation.

Comment on attachment 8498485 [details] [diff] [review] Part 1: Remove text array Review of attachment 8498485 [details] [diff] [review]: ----------------------------------------------------------------- Out of curiosity, 1) this shouldn't have an impact on performance, as the switch should be big enough that the compiler decides to use a jump table? 2) this is done to avoid implicit casts between uint32 and TraceLogger::TextId? ::: js/src/vm/TraceLogging.cpp @@ +79,5 @@ > > TraceLogging traceLoggers; > > +const char * > +TLTextIdString(TraceLogger::TextId id) unless it's used externally in other patches, could you make it static please? @@ +90,2 @@ > #undef NAME > + default: I'd do even better: remove the default case, and move the MOZ_CRASH out of the switch. This way, if one misses a case (shouldn't happen, as the macro should generate all cases anyways), there'll be a warning emitted by the compiler. @@ +90,4 @@ > #undef NAME > + default: > + MOZ_CRASH(); > + return "TraceLogger internal"; I think MOZ_CRASH crashes debug and opt builds, so this is unreachable. @@ +161,5 @@ > fprintf(stderr, "TraceLogging: Error while writing.\n"); > > // Eagerly create the default textIds, to match their Tracelogger::TextId. > for (uint32_t i = 0; i < LAST; i++) { > + TraceLogger::TextId id = static_cast<TraceLogger::TextId>(i); The C++-y form should be (here and a few other places in the patch) TraceLogger::TextId id(i);

Comment on attachment 8498486 [details] [diff] [review] Part 2: Make textId a 'global' type Review of attachment 8498486 [details] [diff] [review]: ----------------------------------------------------------------- r+ if you agree about both the enum renaming and the enum options renamings. The patch should just consist in renaming TraceLogger:: to TraceLogger_ instead, so that should be rather easy. ::: js/src/vm/TraceLogging.h @@ +154,5 @@ > _(GenerateCode) \ > > +// Predefined IDs for common operations. These IDs can be used > +// without using TraceLogCreateTextId, because they are already created. > +enum TL_TextId { It's unusual to have an underscore in the name of the enum, according to a quick glance over a few global enums in the spidermonkey codebase. Could it be named TraceLoggerTextId instead? (no matter how long is the name, as you're a king at vim and you have a great auto-completion engine :D) The same way, it would be ideally better to have a long prefix TraceLogger rather than TL, so that people don't have to remember what the acronym stands for.

Comment on attachment 8498493 [details] [diff] [review] Part 3: Split graph creation out of logging. Review of attachment 8498493 [details] [diff] [review]: ----------------------------------------------------------------- Asking r? for the moz.build changes.

Comment on attachment 8498493 [details] [diff] [review] Part 3: Split graph creation out of logging. Review of attachment 8498493 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, that could be a great opportunity for renaming things: - TraceLogger -> TraceLoggerThread - TraceLogging -> TraceLoggerThreadState - TraceLoggerGraph can stay the same - TraceLoggingGraph -> TraceLoggerGraphState Not r+ yet, because I need to internalize this even more and have a few questions and enough remarks that I would like to see another version of this patch before marking it r+. It is unclear to me what the responsibilities of TraceLoggerGraph are: in my head, it sounds like a TLGraph is supposed to only hold start/stop/timestamp values; now, there are so many events that we need to write them on disk as we are logging them, so that justifies bounding the files to the TraceLoggerGraph itself. Am I understanding this correctly? Also, it seems we can get rid of the |events| variable in TraceLogger (soon to be renamed TraceLoggerThread), see below and tell me if that makes sense. ::: js/src/vm/TraceLogging.cpp @@ +19,1 @@ > #include "vm/Runtime.h" Yeah, make sure to fix that to avoid include ordering failures. @@ +40,2 @@ > { > + uint64_t start = rdtsc() - traceLoggers.startupTime; could you move this just next to the graph.init(start) call, so that timing is more precise? (alternatively, you could move the graph.init just below) @@ +85,5 @@ > if (failed) > return false; > > + // TODO: Remove this. This is so the refactor works with mimimal changes, > + // It is the intention to remove this by logging TL_Enable/TL_Disable. (as stated on irc, should be modified by the following patches) @@ +181,4 @@ > > + uint64_t time = rdtsc() - traceLoggers.startupTime; > + graph.disable(time); > + nit: trailing whitespace @@ +207,5 @@ > + char *str = js_pod_malloc<char>(len + 1); > + if (!str) > + return TL_Error; > + > + DebugOnly<size_t> ret = JS_snprintf(str, len + 1, "%s", text); Why not strcpy? @@ +223,5 @@ > return textId; > } > > uint32_t > +TraceLogger::createTextId(const char *filename, size_t lineno, size_t colno, const void *ptr) Am I understanding correctly that this function is only called for scripts text id, while the one above is called for non-script text ids? It looks like this one could call the other one, in that case (by having the checks for TL_Scripts and constructing the string, and passing that string to the other function). @@ +278,5 @@ > + > +void > +TraceLogger::startEvent(uint32_t id) > +{ > + JS_ASSERT(TLTextIdIsTreeEvent(id)); nit: here and below, MOZ_ASSERT @@ +294,5 @@ > + return; > + > + logTimestamp(TL_Stop); > +} > + nit: trailing whitespace @@ +316,5 @@ > + entryStart.textId = TL_Internal; > + > + EventEntry &entryStop = events.pushUninitialized(); > + entryStop.time = rdtsc() - traceLoggers.startupTime; > + entryStop.textId = TL_Stop; Couldn't this be done in TraceLoggerGraph instead (by say TraceLoggerGraph::logFlushEvents(start, end)), so that we get rid of the entire variable "events" in TraceLogger, and thus make EventEntry a member of TraceLoggerGraph? ::: js/src/vm/TraceLogging.h @@ +84,3 @@ > > PointerHashMap pointerMap; > + Vector<char *, 0, js::SystemAllocPolicy > extraText; nit: no space after SystemAllocPolicy If I understand correctly, this one is used for saving extra text ids, how about naming it extraTextId, or even better, scriptsTextId? ::: js/src/vm/TraceLoggingGraph.cpp @@ +47,5 @@ > + > +uint32_t > +TraceLoggingGraph::nextLoggerId() > +{ > +// TODO: lock as said on IRC, one of the next patches will fix this TODO. @@ +50,5 @@ > +{ > +// TODO: lock > + if (!ensureInitialized()) { > + fprintf(stderr, "TraceLogging: Couldn't create the main log file."); > + return numLoggers++; Shouldn't this function return a particular value if there's an error, like -1? @@ +66,5 @@ > + return numLoggers++; > + } > + } > + > + int written = fprintf(out, "{\"tree\":\"tl-tree.%d.tl\", \"events\":\"tl-event.%d.tl\", \"dict\":\"tl-dict.%d.json\", \"treeFormat\":\"64,64,31,1,32\"}", nit: >99 chars @@ +250,5 @@ > + failed = true; > + return; > + } > + } > + nit: blank line @@ +283,5 @@ > + // child. > + StackEntry &parent = getActiveAncestor(); > +#ifdef DEBUG > + TreeEntry entry; > + if (!getTreeEntry(parent.treeId(), &entry)) Shouldn't this be a MOZ_ALWAYS_TRUE(getTreeEntry(parent.treeId(), &entry)); ? @@ +470,5 @@ > + > +void > +TraceLoggerGraph::disable(uint64_t timestamp) > +{ > + JS_ASSERT(enabled); nit: MOZ_ASSERT @@ +500,5 @@ > + if (failed) > + return; > + > + // Assume ids are given in order. Which is currently true. > + MOZ_ASSERT(id == nextTextId++); Wait, this debug-only side-effect is acceptable because TraceLoggerGraph::nextTextId is only used for this debugging purpose, right? If that's the case, could you make it DebugOnly please and name it expectedNextTextId? ::: js/src/vm/TraceLoggingGraph.h @@ +31,5 @@ > + * => 64 bits: Time Stamp Counter of start of event. > + * => 64 bits: Time Stamp Counter of end of event. > + * => 31 bits: Index to dict file containing the log text. > + * => 1 bit: Boolean signifying if this entry has children. > + * When true, the child can be found just behind this entry. "behind" doesn't really make sense here, maybe "the child can be found right thereafter"? (sorry, was re-reading this comment as it's always a great reminder) @@ +70,5 @@ > + : numLoggers(0), > + out(nullptr) > + { } > + > + bool ensureInitialized(); ensureInitialized is only called by nextLoggedId, could it be private? @@ +124,5 @@ > + void setStop(uint64_t stop) { > + stop_ = stop; > + } > + void setTextId(uint32_t textId) { > + MOZ_ASSERT(textId < uint32_t(1<<31) ); nit: spaces between << operands: 1 << 31 ::: js/src/vm/TraceLoggingTypes.h @@ +52,5 @@ > + result = result|lower; > + > + return result; > +} > +#endif This entire block defining rdtsc is only used in TraceLogging.cpp, and it appears it doesn't need to be exposed, so it should stay there (the fact that functions are static confirms that hint). @@ +133,5 @@ > + } > +} > + > +inline bool > +TLTextIdIsToggable(uint32_t id) okay to do it in a follow-up, but could you use TraceLogger_TextId here as the parameter, and a switch in this function? It should be sane to use TraceLogger_TextId here, as all callers call before any new textId has been added. @@ +160,5 @@ > + return (id > TL_Error && id < TL_LAST_TREE_ITEM) || id >= TL_LAST; > +} > + > +template <class T> > +class ContinuousSpace { Also a good follow-up opportunity, could you make it a standalone class available in mfbt?

Comment on attachment 8498767 [details] [diff] [review] Part 4: Free the things! Review of attachment 8498767 [details] [diff] [review]: ----------------------------------------------------------------- Ogod, we're so used to garbage collectors... Feel free to squash this one with the previous big one.

Comment on attachment 8498493 [details] [diff] [review] Part 3: Split graph creation out of logging. Review of attachment 8498493 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/moz.build @@ +312,5 @@ > > if CONFIG['ENABLE_TRACE_LOGGING']: > SOURCES += [ > 'vm/TraceLogging.cpp', > + 'vm/TraceLoggingGraph.cpp', No need for a build system peer to review that.

Comment on attachment 8498800 [details] [diff] [review] Part 6: Add locking logic Review of attachment 8498800 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TraceLoggingGraph.h @@ +243,5 @@ > void addTextId(uint32_t id, const char *text); > void log(ContinuousSpace<EventEntry> &events); > }; > > +class AutoTraceLoggingGraphLock Please move it in TraceloggingGraph.cpp where the only use is

Comment on attachment 8498783 [details] [diff] [review] Part 5: Don't clear the log list upon disable/enable. Review of attachment 8498783 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for now, as stated on irc.

Comment on attachment 8498834 [details] [diff] [review] Part 7: Disable graph creation by default Review of attachment 8498834 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review for now, as stated on irc.

Comment on attachment 8498493 [details] [diff] [review] Part 3: Split graph creation out of logging. Review of attachment 8498493 [details] [diff] [review]: ----------------------------------------------------------------- Answering your questions on part 3. I adjusted everything, except otherwise stated here. (I'll upload later) ::: js/src/vm/TraceLogging.cpp @@ +207,5 @@ > + char *str = js_pod_malloc<char>(len + 1); > + if (!str) > + return TL_Error; > + > + DebugOnly<size_t> ret = JS_snprintf(str, len + 1, "%s", text); I think I created the "script" variant first, which alters the output. As a result this also uses JS_snprintf. I don't think it make a difference, so I kept JS_snprintf. @@ +223,5 @@ > return textId; > } > > uint32_t > +TraceLogger::createTextId(const char *filename, size_t lineno, size_t colno, const void *ptr) There are a few issues with merging both. Which would be better. createTextId(const char *) assumes the char * needs to be copied. While in createTextId(JSScript *script), we just constructed the string, so can be owned. Secondly we test if we have a hit before copying the string. This is a bit annoying for createTextId(JSScript *script) to call createTextId(const char *), since in that case it will need to create the string beforehand. In other words I tried, but there isn't really a good way to nicely merge them. @@ +316,5 @@ > + entryStart.textId = TL_Internal; > + > + EventEntry &entryStop = events.pushUninitialized(); > + entryStop.time = rdtsc() - traceLoggers.startupTime; > + entryStop.textId = TL_Stop; Oh no. We cannot remove "events" in TraceLogger at all. TraceLoggerThread puts all logged events in "events". - Debugger will read 'events' - TraceLoggerGraph will read 'events' and create a graph out of it in a file (with a temp. storage in ContinuousSpace). So 'events' is the source of all output. (The other way around is possible. We can remove the EventEntry out of TraceLoggerGraph) ::: js/src/vm/TraceLogging.h @@ +84,3 @@ > > PointerHashMap pointerMap; > + Vector<char *, 0, js::SystemAllocPolicy > extraText; scriptsTextId is a bit overrestrictive. We can have other textIds, which are not related to scripts in here. ::: js/src/vm/TraceLoggingGraph.cpp @@ +283,5 @@ > + // child. > + StackEntry &parent = getActiveAncestor(); > +#ifdef DEBUG > + TreeEntry entry; > + if (!getTreeEntry(parent.treeId(), &entry)) No. getTreeEntry can fail. Part of the graph can already be on disk. So retrieving it from disk might fail. In that case we should fail "gracefully"

Comment on attachment 8504935 [details] [diff] [review] Part 1.5: Fix use of JitFrameIterator Review of attachment 8504935 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TraceLogging.cpp @@ +210,2 @@ > > MOZ_ASSERT(it.type() == JitFrame_Exit); This assertion is no longer holds, you might want to replace all the code which is below by: while (!it.isScripted() && !it.done()) it++; The reason being that the top-level Jit frame might be bailing out.

Comment on attachment 8504940 [details] [diff] [review] Part 3: Split graph creation out of logging. Review of attachment 8504940 [details] [diff] [review]: ----------------------------------------------------------------- Wow, thanks for the renaming and good job on the patch! Mostly nits and comments (which are answers to questions i've privately asked on irc), so r+ing now to not block you on this. It seems that createTextId(const char*) is only used with an empty string, so we might want to create createEmptyTextId instead, or generalize createTextId, i don't know. Or keep it, as you just said on irc that we could actually create text id for anything. I suspect that could be a thing in the debugger in the future. ::: js/src/vm/TraceLogging.cpp @@ +112,4 @@ > return false; > > + // Minimum amount of capacity needed for operation. > + if (!events.ensureSpaceBeforeAdd(3)) can you explicit the above comment, saying that when we flush events, we 1) create a new event entry for TL internal 2) create a new stop event entry 3) create the event entry for the actual event @@ +272,5 @@ > + > + DebugOnly<size_t> ret = JS_snprintf(str, len + 1, "%s", text); > + MOZ_ASSERT(ret == len); > + > + uint32_t textId = extraTextId.length() + TraceLogger_Last; could you move it closer to its first use, next to the if (!pointerMap...) below? @@ +306,5 @@ > + for (size_t i = lineno; i /= 10; lenLineno++); > + size_t lenColno = 1; > + for (size_t i = colno; i /= 10; lenColno++); > + > + size_t len = 7 + lenFilename + 1 + lenLineno + 1 + lenColno; can you add a comment above: // The string below explicits the length requirements. @@ +317,2 @@ > > + uint32_t textId = extraTextId.length() + TraceLogger_Last; can you move it down here too? ::: js/src/vm/TraceLogging.h @@ +77,3 @@ > FILE *dictFile; > FILE *treeFile; > FILE *eventFile; as discussed on IRC, these should be gone in TLGraph now. Please remove them. ::: js/src/vm/TraceLoggingGraph.cpp @@ +78,5 @@ > + return numLoggers++; > +} > + > +bool > +TraceLoggerGraph::init(uint64_t timestamp) can you rename this param into startTimestamp or startTime or something along these lines please? @@ +80,5 @@ > + > +bool > +TraceLoggerGraph::init(uint64_t timestamp) > +{ > + if (!events.init()) as discussed on irc, some or all these failures could benefit from failed_ = true; @@ +88,5 @@ > + if (!stack.init()) > + return false; > + > + uint32_t loggerId = traceLoggersGraph.nextLoggerId(); > + if (loggerId > 999) could you instead test that loggerId != uint32_t(-1))? (so that the 999 value stays encapsulated in the nextLoggerId method) @@ +117,5 @@ > + treeFile = nullptr; > + return false; > + } > + > + TreeEntry &treeEntry = tree.pushUninitialized(); Could you add a comment above that these are the top tree and stack entries? @@ +131,5 @@ > + stackEntry.setActive(true); > + > + int written = fprintf(dictFile, "["); > + if (written < 0) > + fprintf(stderr, "TraceLogging: Error while writing.\n"); can you return false in this case? @@ +382,5 @@ > +TraceLoggerGraph::getTreeEntry(uint32_t treeId, TreeEntry *entry) > +{ > + // Entry is still in memory > + if (treeId >= treeOffset) { > + *entry = tree[treeId]; shouldn't this be tree[treeId - treeOffset]? ::: js/src/vm/TraceLoggingGraph.h @@ +62,5 @@ > + > +class TraceLoggerGraphState > +{ > + size_t numLoggers; > + FILE *out; could you add a comment that this "out" file is the output of a tracelogging session as described in the above comment, please? @@ +87,5 @@ > + uint64_t stop_; > + union { > + struct { > + uint32_t textId_: 31; > + uint32_t hasChildren_: 1; can you make hasChildren a bool? @@ +126,5 @@ > + void setStop(uint64_t stop) { > + stop_ = stop; > + } > + void setTextId(uint32_t textId) { > + MOZ_ASSERT(textId < uint32_t(1 << 31) ); nit: space before closing parenthesis @@ +145,5 @@ > + uint32_t treeId_; > + uint32_t lastChildId_; > + struct { > + uint32_t textId_: 31; > + uint32_t active_: 1; can you make active_ a bool? @@ +172,5 @@ > + void setLastChildId(uint32_t lastChildId) { > + lastChildId_ = lastChildId; > + } > + void setTextId(uint32_t textId) { > + MOZ_ASSERT(textId < uint32_t(1<<31) ); nit: space before closing parenthesis @@ +196,5 @@ > + private: > + bool failed; > + bool enabled; > +#ifdef DEBUG > + uint32_t nextTextId; can you make it mozilla::DebugOnly<uint32_t>, to avoid the #ifdef DEBUG here and in the ctor? @@ +244,5 @@ > + public: > + void addTextId(uint32_t id, const char *text); > + void log(ContinuousSpace<EventEntry> &events); > + void disable(uint64_t timestamp); > + void enable(); can you group all public functions in the same place (there are a few around the ctor, above)? ::: js/src/vm/TraceLoggingTypes.h @@ +82,5 @@ > + TRACELOGGER_LOG_ITEMS(NAME) > +#undef NAME > + default: > + MOZ_CRASH(); > + return "TraceLogger internal"; nit: unreachable, please remove this statement @@ +96,5 @@ > + return false; > + if (id == TraceLogger_Stop) > + return false; > + // Actually never used. But added here so it doesn't show as toggle > + if (id == TraceLogger_LastTreeItem) could you also MOZ_ASSERT(id != TraceLogger_Last)?

Comment on attachment 8504945 [details] [diff] [review] Part 5: Don't clear the log list upon disable/enable. Review of attachment 8504945 [details] [diff] [review]: ----------------------------------------------------------------- This means that enabling and disabling traceloggers will be logged as one-time events as well. It makes sense for visualizers like the web reporting thing. Nice.

Comment on attachment 8504948 [details] [diff] [review] Part 7: Disable graph creation by default Review of attachment 8504948 [details] [diff] [review]: ----------------------------------------------------------------- r+ with UniquePtr, which will avoid to explicitly delete. ::: js/src/vm/TraceLogging.cpp @@ +119,5 @@ > > +void > +TraceLoggerThread::initGraph() > +{ > + graph = new TraceLoggerGraph(); js_new? @@ +126,5 @@ > + > + uint64_t start = rdtsc() - traceLoggers.startupTime; > + if (!graph->init(start)) { > + delete graph; > + graph = nullptr; don't forget to force return here, if you don't want a segfault three lines below ;) ::: js/src/vm/TraceLogging.h @@ +80,5 @@ > > uint32_t enabled; > bool failed; > > + TraceLoggerGraph *graph; Seems like UniquePtr's perfect use case here. http://dxr.mozilla.org/mozilla-central/source/mfbt/UniquePtr.h @@ +153,5 @@ > bool enabled; > bool enabledTextIds[TraceLogger_Last]; > bool mainThreadEnabled; > bool offThreadEnabled; > + bool graphEnabled; could it be graphSpewingEnabled, or graphWritingEnabled, or something like this?

(In reply to Benjamin Bouvier [:bbouvier] from comment #31) > @@ +382,5 @@ > > +TraceLoggerGraph::getTreeEntry(uint32_t treeId, TreeEntry *entry) > > +{ > > + // Entry is still in memory > > + if (treeId >= treeOffset) { > > + *entry = tree[treeId]; > > shouldn't this be tree[treeId - treeOffset]? good find! > @@ +87,5 @@ > > + uint64_t stop_; > > + union { > > + struct { > > + uint32_t textId_: 31; > > + uint32_t hasChildren_: 1; > > can you make hasChildren a bool? > @@ +145,5 @@ > > + uint32_t treeId_; > > + uint32_t lastChildId_; > > + struct { > > + uint32_t textId_: 31; > > + uint32_t active_: 1; > > can you make active_ a bool? Since a bool isn't one bit, but 1 char, this wouldn't give use the 31 bits and 1 bit cut like we expect ;). So we can't do this. > @@ +196,5 @@ > > + private: > > + bool failed; > > + bool enabled; > > +#ifdef DEBUG > > + uint32_t nextTextId; > > can you make it mozilla::DebugOnly<uint32_t>, to avoid the #ifdef DEBUG here > and in the ctor? I didn't know DebugOnly worked this way. Nice. > @@ +96,5 @@ > > + return false; > > + if (id == TraceLogger_Stop) > > + return false; > > + // Actually never used. But added here so it doesn't show as toggle > > + if (id == TraceLogger_LastTreeItem) > > could you also MOZ_ASSERT(id != TraceLogger_Last)? I added TraceLogger_Last as non-toggable.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f206e54f843e https://hg.mozilla.org/integration/mozilla-inbound/rev/971439e9c318 https://hg.mozilla.org/integration/mozilla-inbound/rev/8661c6e4fa51 https://hg.mozilla.org/integration/mozilla-inbound/rev/33167a292285 https://hg.mozilla.org/integration/mozilla-inbound/rev/8b9390d62ecd https://hg.mozilla.org/integration/mozilla-inbound/rev/5485b455c029 https://hg.mozilla.org/integration/mozilla-inbound/rev/7d0eb79dd247 https://hg.mozilla.org/integration/mozilla-inbound/rev/8d040e2b9aff

https://hg.mozilla.org/mozilla-central/rev/8d040e2b9aff https://hg.mozilla.org/mozilla-central/rev/7d0eb79dd247 https://hg.mozilla.org/mozilla-central/rev/5485b455c029 https://hg.mozilla.org/mozilla-central/rev/8b9390d62ecd https://hg.mozilla.org/mozilla-central/rev/33167a292285 https://hg.mozilla.org/mozilla-central/rev/8661c6e4fa51 https://hg.mozilla.org/mozilla-central/rev/971439e9c318 https://hg.mozilla.org/mozilla-central/rev/f206e54f843e

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/16aa61577e95 https://hg.mozilla.org/mozilla-central/rev/37770c7a3f0f https://hg.mozilla.org/mozilla-central/rev/63b734ac95b2 https://hg.mozilla.org/mozilla-central/rev/12348c397b08 https://hg.mozilla.org/mozilla-central/rev/04988ab0c1da https://hg.mozilla.org/mozilla-central/rev/aad64d6ad822 https://hg.mozilla.org/mozilla-central/rev/20dc3d4a9303 https://hg.mozilla.org/mozilla-central/rev/2d0b50d68320