Last bits to have a successful first integration of TraceLogger in Debugger

Currently the debugger doesn't get timing information. (rdtsc is not universal, so should get replaced for wider audience.) So no need to measure it during logging (if graph is disabled).

Make a difference between "start script" and "annotate script". Where the first means, we entered that function for execution. While the later is to annotate a Compiling/parsing/... event with a particular script.

For traces we want to log inlined scripts. So make it possible to toggle that on. Currently disabling inlining in IonMonkey to get this. (I felt it was too risky to support reporting inlined frames for now. I opened a follow-up bug to do this when everything has landed and first load of bugs are fixed).

Currently "TLThread::extraTextId" maps "textId to "log text". Now currently this is a vector. As a result the size of this increases by using it and will fail if it gets too big. In order to fix this, we need to know which "textIds" are still used. Unused "textIds" can get removed in that way. TraceLoggerEventPayload is a triple containing "textId", "log text" and "uses". When uses drops to zero, we know we can remove that payload. TraceLoggerEvent is a class used to create such a payload and automatically do the increasing/decreasing of payload uses.

Comment on attachment 8508060 [details] [diff] [review] Part 2: Add AnnotateScripts Review of attachment 8508060 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure I understand this change well enough to r+ it. Can you give a bit more background on this? In particular, it seems like the only places where Scripts instead of AnnotateScripts is now used are in baseline. What makes those usages special compared to the bytecode compiler, interpreter and Ion? ::: js/src/vm/TraceLogging.cpp @@ +320,5 @@ > filename = "<unknown>"; > > assertNoQuotes(filename); > > // Only log scripts when enabled otherwise return the global Scripts textId, This comment needs slight updating. Perhaps "return the generic textId that was passed in, which [..]"?

Comment on attachment 8508084 [details] [diff] [review] Part 6: use js_ for new and delete Review of attachment 8508084 [details] [diff] [review]: ----------------------------------------------------------------- Oh, absolutely.

Comment on attachment 8508085 [details] [diff] [review] Part 7: Make --disable-trace-logging work again. Review of attachment 8508085 [details] [diff] [review]: ----------------------------------------------------------------- Nice

(In reply to Till Schneidereit [:till] from comment #9) > Comment on attachment 8508060 [details] [diff] [review] > Part 2: Add AnnotateScripts > > Review of attachment 8508060 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm not sure I understand this change well enough to r+ it. Can you give a > bit more background on this? In particular, it seems like the only places > where Scripts instead of AnnotateScripts is now used are in baseline. What > makes those usages special compared to the bytecode compiler, interpreter > and Ion? Scripts is still used in Interpreter, Baseline and IonMonkey (and Odinmonkey still needs to get added). It means the start of a function. So everytime we enter a function "Scripts" will get logged. AnnotateScripts is not the start of a function. But gives extra information about which scripts we are talking. E.g. when starting a parsing job, when starting a baseline/ionmonkey compilation job ...

Comment on attachment 8508062 [details] [diff] [review] Part 3: Remove use of EventEntry from TLGraph Review of attachment 8508062 [details] [diff] [review]: ----------------------------------------------------------------- Couldn't we just poll events from TLThread, now that it has a public getEvents() method? I am worried about the impact on I/O this patch will have. Have you made some measurements to see impact on speed + made sure that TLGraph doesn't get disabled too early? ::: js/src/vm/TraceLoggingGraph.cpp @@ +401,5 @@ > + id = NativeEndian::swapToBigEndian(id); > + > + // The layout of the event log in the log file is: > + // [timestamp, textId] > + size_t bytesWritten = 0; It doesn't count bytes but the number of variables actually written down on disk. numWrittenFields? numWrittenVars? @@ +407,5 @@ > + bytesWritten += fwrite(&id, sizeof(uint32_t), 1, eventFile); > + if (bytesWritten < 2) { > + failed = true; > + enabled = false; > + return; this return has no effect

Comment on attachment 8508065 [details] [diff] [review] Part 4: Make it possible to toggle if inlining scripts need to get reported Review of attachment 8508065 [details] [diff] [review]: ----------------------------------------------------------------- Fine as long as it's temporary. Observing code should have minimal effect on the observee. ::: js/src/jit/IonBuilder.cpp @@ +351,5 @@ > return InliningDecision_DontInline; > > + if (TraceLogTextIdEnabled(TraceLogger_InlinedScripts)) { > + return DontInline(nullptr, "TraceLogging inlined scripts is enabled, " > + "which tracelogger cannot yet"); can you change it to Tracelogging of inlined scripts is enabled but Tracelogger cannot do that yet.

Comment on attachment 8508060 [details] [diff] [review] Part 2: Add AnnotateScripts Review of attachment 8508060 [details] [diff] [review]: ----------------------------------------------------------------- Ok, with your explanation, that makes sense. The Scripts instead of AnnotateScripts threw me off, and it seems like it really should be AnnotateScripts? In any case: r=me with comment fixed and that bailout case changed or explained away. ::: js/src/jit/BaselineBailouts.cpp @@ +1410,5 @@ > // Skip recover instructions as they are already recovered by |initInstructionResults|. > snapIter.settleOnFrame(); > > if (frameNo > 0) { > + TraceLogStartEvent(logger, TraceLogCreateTextId(logger, TraceLogger_Scripts, scr)); Shouldn't this use AnnotateScripts?

Comment on attachment 8508053 [details] [diff] [review] Part 1: Don't log time for debugger Review of attachment 8508053 [details] [diff] [review]: ----------------------------------------------------------------- That should be a choice of the consumer to do that, but as TLGraph is tied to the TLThread at the moment, it's fine to do that. ::: js/src/vm/TraceLogging.cpp @@ +419,5 @@ > entryStop.textId = TraceLogger_Stop; > } > } > > + uint64_t time = (graph) ? rdtsc() - traceLoggers.startupTime : 0; Would it make sense to have this entire code conditioned by graph? this way, we don't push an empty entry with time == 0 for the debugger.

Like explained on IRC, this won't have an impact on TLGraph. 1) IO is already buffered by OS. 2) we only emit the one time events. Bailouts/Enable/Disable. The tree events are still buffered.

Comment on attachment 8508078 [details] [diff] [review] Part 5: Add TraceLoggerEvent class Review of attachment 8508078 [details] [diff] [review]: ----------------------------------------------------------------- First round of comments, would like that some reviewer see an updated version later. ::: js/src/jit/BaselineCompiler.cpp @@ +774,5 @@ > > // Script start. > + masm.movePtr(ImmGCPtr(script), scriptReg); > + masm.loadPtr(Address(scriptReg, JSScript::offsetOfBaselineScript()), scriptReg); > + masm.lea(Operand(scriptReg, BaselineScript::offsetOfTraceLoggerScriptEvent()), scriptReg); can you use loadPtr(Address, scriptReg) as well here? ::: js/src/jit/BaselineJIT.cpp @@ +837,5 @@ > +#endif > + > + TraceLoggerThread *logger = TraceLoggerForMainThread(runtime); > + if (TraceLogTextIdEnabled(TraceLogger_Scripts)) { > + TraceLoggerEvent event(logger, TraceLogger_Scripts, script); here and below, no need for the temporary: traceLoggerScriptEvent_ = TraceLoggerEvent(...); ::: js/src/jit/IonMacroAssembler.cpp @@ +1468,5 @@ > + > + setupUnalignedABICall(2, temp); > + passABIArg(logger); > + passABIArg(event); > + callWithABINoProfiling(JS_FUNC_TO_DATA_PTR(void *, TraceLogFunc)); Can you just inline the use of TraceLogStartEvent here, avoiding the TraceLogFunc def? ::: js/src/vm/TraceLogging.cpp @@ +297,5 @@ > + > + TraceLoggerEventPayload *payload = js_new<TraceLoggerEventPayload>(textId, nullptr); > + > + if (!extraTextId.add(p, textId, payload)) > + return getOrCreateEventPayload(TraceLogger_Error); as you return a pointer in this function and the one below, would it make sense to return nullptr and check for this special value on the callers' sides, to avoid having the TL_Error enum? @@ +329,5 @@ > + if (!extraTextId.putNew(textId, payload)) > + return getOrCreateEventPayload(TraceLogger_Error); > + > + if (!pointerMap.add(p, text, payload)) > + return getOrCreateEventPayload(TraceLogger_Error); please group the if statements into one single if() @@ +382,5 @@ > + if (!extraTextId.putNew(textId, payload)) > + return getOrCreateEventPayload(TraceLogger_Error); > + > + if (!pointerMap.add(p, ptr, payload)) > + return getOrCreateEventPayload(TraceLogger_Error); can you group the three checks in one single if() statement please? @@ +410,5 @@ > + startEvent(uint32_t(id)); > +} > + > +void > +TraceLoggerThread::startEvent(TraceLoggerEvent *event) { That'd be *so nice* for users to have a const TraceLoggerEvent &event, here instead. ::: js/src/vm/TraceLogging.h @@ +75,5 @@ > + TraceLoggerEvent (TraceLoggerThread *logger, TraceLoggerTextId type, JSScript *script); > + TraceLoggerEvent (TraceLoggerThread *logger, TraceLoggerTextId type, > + const JS::ReadOnlyCompileOptions &compileOptions); > + TraceLoggerEvent (TraceLoggerThread *logger, const char *text); > + TraceLoggerEvent () { payload_ = nullptr; }; nit: here and above, remove the spaces before opening ( @@ +85,5 @@ > + } > +}; > + > +class TraceLoggerEventPayload { > + public: (you could make it a struct) @@ +87,5 @@ > + > +class TraceLoggerEventPayload { > + public: > + uint32_t textId; > + char *payload; Payload in the class name and as a field member is confusing. How about label? Plus, UniquePtr<char*> to avoid the explicit free in the TLThread dtor? @@ +88,5 @@ > +class TraceLoggerEventPayload { > + public: > + uint32_t textId; > + char *payload; > + uint32_t uses; can you add a \n before the ctor please? @@ +98,5 @@ > + > + void use() { > + uses++; > + } > + void free() { release()? @@ +365,5 @@ > + union { > + TraceLoggerEvent *event; > + TraceLoggerTextId id; > + } payload; > + bool event; bool isEvent;

Thanks all for the reviews! This is mostly ready to land. Looking at it, I'm gonna land/rerequest review part 5 afterwards. Making it is easier to review.

(In reply to Benjamin Bouvier [:bbouvier] from comment #16) > Comment on attachment 8508053 [details] [diff] [review] > Part 1: Don't log time for debugger > > Review of attachment 8508053 [details] [diff] [review]: > ----------------------------------------------------------------- > > That should be a choice of the consumer to do that, but as TLGraph is tied > to the TLThread at the moment, it's fine to do that. > > ::: js/src/vm/TraceLogging.cpp > @@ +419,5 @@ > > entryStop.textId = TraceLogger_Stop; > > } > > } > > > > + uint64_t time = (graph) ? rdtsc() - traceLoggers.startupTime : 0; > > Would it make sense to have this entire code conditioned by graph? this way, > we don't push an empty entry with time == 0 for the debugger. An entry is "time + textId". So we aren't pushing empty entries when graph is disabled. We only push the "textId" in that case (with time 0).

(In reply to Till Schneidereit [:till] from comment #15) > Comment on attachment 8508060 [details] [diff] [review] > Part 2: Add AnnotateScripts > > Review of attachment 8508060 [details] [diff] [review]: > ----------------------------------------------------------------- > > Ok, with your explanation, that makes sense. The Scripts instead of > AnnotateScripts threw me off, and it seems like it really should be > AnnotateScripts? In any case: r=me with comment fixed and that bailout case > changed or explained away. > ... and it seems like it really should be AnnotateScripts? ... Not sure I understand your remark here. I use AnnotateScripts right and you ask me to change AnnotateScripts with AnnotateScripts. A bit confused here. > ::: js/src/jit/BaselineBailouts.cpp > @@ +1410,5 @@ > > // Skip recover instructions as they are already recovered by |initInstructionResults|. > > snapIter.settleOnFrame(); > > > > if (frameNo > 0) { > > + TraceLogStartEvent(logger, TraceLogCreateTextId(logger, TraceLogger_Scripts, scr)); > > Shouldn't this use AnnotateScripts? Actually no. This is a peculiar thing. In IonMonkey we don't keep track of inlined frames. So we don't create events in TL for them. Though in Baseline we do. So when bailing from IM to Baseline, we create these events, to make sure the state is correct in BaseLine. I added a comment in the code.

(In reply to Benjamin Bouvier [:bbouvier] from comment #18) > ::: js/src/jit/BaselineCompiler.cpp > @@ +774,5 @@ > > > > // Script start. > > + masm.movePtr(ImmGCPtr(script), scriptReg); > > + masm.loadPtr(Address(scriptReg, JSScript::offsetOfBaselineScript()), scriptReg); > > + masm.lea(Operand(scriptReg, BaselineScript::offsetOfTraceLoggerScriptEvent()), scriptReg); > > can you use loadPtr(Address, scriptReg) as well here? It should be computeEffectiveAddress. BaselineScript has TraceLoggerEvent, not a reference (TraceLoggerEvent *). So we need to have the address where TraceLoggerEvent starts in BaselineScript. > ::: js/src/jit/IonMacroAssembler.cpp > @@ +1468,5 @@ > > + > > + setupUnalignedABICall(2, temp); > > + passABIArg(logger); > > + passABIArg(event); > > + callWithABINoProfiling(JS_FUNC_TO_DATA_PTR(void *, TraceLogFunc)); > > Can you just inline the use of TraceLogStartEvent here, avoiding the > TraceLogFunc def? Not really. TraceLogStartEvent has now two different signatures (TraceLoggerTextId and TraceLoggerEvent). So it needs to know which one it needs to inline. > @@ +329,5 @@ > > + if (!extraTextId.putNew(textId, payload)) > > + return getOrCreateEventPayload(TraceLogger_Error); > > + > > + if (!pointerMap.add(p, text, payload)) > > + return getOrCreateEventPayload(TraceLogger_Error); > > please group the if statements into one single if() Not really possible anymore. With the added js_delete/js_free things. > @@ +85,5 @@ > > + } > > +}; > > + > > +class TraceLoggerEventPayload { > > + public: > > (you could make it a struct) I didn't do this (yet). Feel free to suggest this again. Not sure when to use class/struct.

With quite some pain I was able to create a git repo out of all patches: https://github.com/h4writer/ssa-to-js/commits/tracelogger2 So this contains all patches I will land for TraceLogger until patch 4. So patch 5/6/7/8 of this bug isn't included. But all other bugs/patches are. Sorry that the commit message isn't set. I hope this makes it a bit easier to review. Since you can navigate the source and see the status quo before this patch.

https://github.com/h4writer/ssa-to-js/tree/tracelogger2 is the main link ;).

(In reply to Hannes Verschore [:h4writer] from comment #21) > > Ok, with your explanation, that makes sense. The Scripts instead of > > AnnotateScripts threw me off, and it seems like it really should be > > AnnotateScripts? In any case: r=me with comment fixed and that bailout case > > changed or explained away. > > > ... and it seems like it really should be AnnotateScripts? ... > > Not sure I understand your remark here. I use AnnotateScripts right and you > ask me to change AnnotateScripts with AnnotateScripts. A bit confused here. Sorry, that was a *very* unclear comment. I meant exactly the thing you explained below, with Baseline bailouts using _Scripts instead of _AnnotateScripts. > Actually no. This is a peculiar thing. In IonMonkey we don't keep track of > inlined frames. So we don't create events in TL for them. Though in Baseline > we do. So when bailing from IM to Baseline, we create these events, to make > sure the state is correct in BaseLine. I added a comment in the code. Thanks, that really helps.

Comment on attachment 8512663 [details] [diff] [review] tracelogger-part5-add-tracelog-event-bug1083694 Review of attachment 8512663 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Most of my comments are nits. Before landing all the patches, please make sure: 1) it builds in non-unified / arch=none builds. 2) all jit-tests pass with TL disabled (but JS_TRACE_LOGGER defined) 3) all jit-tests pass with TL enabled (there could be some bad events stack balancing e.g. which would be hard to find only by reading code). Nice work! ::: js/src/jit/BaselineJIT.cpp @@ +839,5 @@ > + TraceLoggerThread *logger = TraceLoggerForMainThread(runtime); > + if (TraceLogTextIdEnabled(TraceLogger_Scripts)) > + traceLoggerScriptEvent_ = TraceLoggerEvent(logger, TraceLogger_Scripts, script); > + else > + traceLoggerScriptEvent_ = TraceLoggerEvent(logger, TraceLogger_Scripts); might be more readable as traceLoggerScriptEvent_ = TraceLoggerEvent(logger, TL_Scripts, TLTextIdEnabled(...) ? script : nullptr); @@ +864,2 @@ > if (enable) { > + TraceLoggerEvent event(logger, TraceLogger_Scripts, script); if you inline the ctor calls here, you can even get rid of the {} ::: js/src/jit/BaselineJIT.h @@ +130,3 @@ > > #ifdef JS_TRACE_LOGGING > #ifdef DEBUG while you're here: can you indent the inner #ifdef by one space after the sharp please? #ifdef JS_TRACE_LOGGING # ifdef DEBUG ... ::: js/src/jit/IonCode.h @@ +494,5 @@ > } > bool hasSPSInstrumentation() const { > return hasSPSInstrumentation_; > } > + void setTraceLoggerEvent(TraceLoggerEvent event) { Ha, copy by value :/ Might be worth to either change this or add a comment that this is fine to do so as TLEvent only contains a pointer. ::: js/src/vm/TraceLogging.cpp @@ +159,5 @@ > graph = nullptr; > } > > + for (TextIdHashMap::Range r = extraTextId.all(); !r.empty(); r.popFront()) { > + js_delete(r.front().value()); nit: single statement in for loop => no {} Crazy idea: could we have the TextIdHashMap be mapping keys to UniquePtr<TLPayload>? (good idea for a follow up or a good first bug) @@ +164,2 @@ > } > + extraTextId.finish(); ooc, why doesn't pointerMap need the same call to finish here? @@ +347,5 @@ > + uint32_t textId = extraTextId.count() + TraceLogger_Last; > + > + TraceLoggerEventPayload *payload = js_new<TraceLoggerEventPayload>(textId, str); > + if (!payload) { > + js_free(payload); did you mean js_free(str) ? ::: js/src/vm/TraceLogging.h @@ +55,5 @@ > + * TraceLoggerTextId. They don't require to create an TraceLoggerEvent and > + * can also be used as an argument to these functions. > + * 3) Log the occurence of a single event: > + * - TraceLogTimestamp(logger, TraceLoggerTextId); > + * Note: it is temporary not supported to provide an TraceLoggerEvent as nit: temporarily (i think) @@ +88,5 @@ > + public: > + TraceLoggerEvent(TraceLoggerThread *logger, TraceLoggerTextId textId); > + TraceLoggerEvent(TraceLoggerThread *logger, TraceLoggerTextId type, JSScript *script); > + TraceLoggerEvent(TraceLoggerThread *logger, TraceLoggerTextId type, > + const JS::ReadOnlyCompileOptions &compileOptions); nit: alignment of args @@ +111,5 @@ > + */ > +class TraceLoggerEventPayload { > + public: > + uint32_t textId; > + mozilla::UniquePtr<char, JS::FreePolicy> string_; shouldn't make string_ a public property if it has an accessor. How about uniformizing all of this and have accessors for all properties? @@ +247,2 @@ > > + friend void TraceLogTimestampPrivate(TraceLoggerThread *logger, uint32_t id); The private logTimestamp/startEvent/stopEvent(uint32_t id) functions are publicly accessible via these TraceLog*Private functions anyways (and except for the comment below, there's no way to prevent using them, it seems), so i'd just remove these friend declarations and have the 3 functions public.

We hit this on try and it is a bit silly to not support quotes in filenames.

Comment on attachment 8518032 [details] [diff] [review] Part 9: Remove use of assertNoQuotes Review of attachment 8518032 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsstr.h @@ +396,5 @@ > } > > +template <typename CharT> > +inline bool > +FileEscapedString(FILE *fp, const CharT *chars, size_t length, uint32_t quote) no need for a template function here, as you only use one specialization for char*. This could be inline bool FileEscapedString(FILE *fp, const char *chars, size_t length, uint32_t quote) ::: js/src/vm/TraceLogging.h @@ +15,5 @@ > > #include "js/HashTable.h" > #include "js/TypeDecls.h" > #include "js/Vector.h" > +#include "vm/TraceLoggingGraph.h" Please make sure to include this change in the patch which introduced the issue.

So after some round of tbpl and fixing issues (which I fixed in the correct patches. Stupid things like (char *) nullptr, changing TraceLogger::IonMonkey into TraceLogger_IonMonkey etc). With this fix we are now green op tbpl :D.

Comment on attachment 8526031 [details] [diff] [review] Part 10: define dummy rdtsc Review of attachment 8526031 [details] [diff] [review]: ----------------------------------------------------------------- Alright. Out of curiosity, which platforms are affected?

(In reply to Benjamin Bouvier [:bbouvier] from comment #30) > Comment on attachment 8526031 [details] [diff] [review] > Part 10: define dummy rdtsc > > Review of attachment 8526031 [details] [diff] [review]: > ----------------------------------------------------------------- > > Alright. Out of curiosity, which platforms are affected? Currently ARM (and MIPS) isn't implemented. I think Marty has a patch somewhere though. Opened bug 1102242 as follow-up.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee7bf79625e https://hg.mozilla.org/integration/mozilla-inbound/rev/54c8f117c3db https://hg.mozilla.org/integration/mozilla-inbound/rev/05ca63f6ffe3 https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5dc516518d https://hg.mozilla.org/integration/mozilla-inbound/rev/aafd058bcab1 https://hg.mozilla.org/integration/mozilla-inbound/rev/83acb541d44a https://hg.mozilla.org/integration/mozilla-inbound/rev/59f15435d8de https://hg.mozilla.org/integration/mozilla-inbound/rev/4ba3f3e635fc https://hg.mozilla.org/integration/mozilla-inbound/rev/aa78b751e325

https://hg.mozilla.org/mozilla-central/rev/aa78b751e325 https://hg.mozilla.org/mozilla-central/rev/4ba3f3e635fc https://hg.mozilla.org/mozilla-central/rev/59f15435d8de https://hg.mozilla.org/mozilla-central/rev/83acb541d44a https://hg.mozilla.org/mozilla-central/rev/aafd058bcab1 https://hg.mozilla.org/mozilla-central/rev/7f5dc516518d https://hg.mozilla.org/mozilla-central/rev/05ca63f6ffe3 https://hg.mozilla.org/mozilla-central/rev/54c8f117c3db https://hg.mozilla.org/mozilla-central/rev/0ee7bf79625e

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

Comment on attachment 8541689 [details] [diff] [review] part 11: Fix tracelogger with infallible codegen Review of attachment 8541689 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/shared/CodeGenerator-shared.cpp @@ +1413,5 @@ > masm.Push(logger); > > CodeOffsetLabel patchLogger = masm.movWithPatch(ImmPtr(nullptr), logger); > if (!patchableTraceLoggers_.append(patchLogger)) > + MOZ_CRASH(); Instead of crashing, you can use masm.propagateOOM here. @@ +1422,5 @@ > masm.Push(script); > > CodeOffsetLabel patchScript = masm.movWithPatch(ImmWord(0), script); > if (!patchableTLScripts_.append(patchScript)) > + MOZ_CRASH(); Ditto. @@ +1450,5 @@ > masm.Push(logger); > > CodeOffsetLabel patchLocation = masm.movWithPatch(ImmPtr(nullptr), logger); > if (!patchableTraceLoggers_.append(patchLocation)) > + MOZ_CRASH(); Ditto.

I pushed this follow-up in order to fix the static-analysis build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c63cf613373

https://hg.mozilla.org/mozilla-central/rev/092b29ea64e0 https://hg.mozilla.org/mozilla-central/rev/0d17765346d5 https://hg.mozilla.org/mozilla-central/rev/f15a85271605 https://hg.mozilla.org/mozilla-central/rev/7091b8b54c91 https://hg.mozilla.org/mozilla-central/rev/c75c3046e1c8 https://hg.mozilla.org/mozilla-central/rev/f2f9d34c115b https://hg.mozilla.org/mozilla-central/rev/15a65e7b59e6 https://hg.mozilla.org/mozilla-central/rev/dff82e9a174b https://hg.mozilla.org/mozilla-central/rev/ef7a85ec6595 https://hg.mozilla.org/mozilla-central/rev/1c63cf613373