Closed Bug 1083694 Opened 10 years ago Closed 9 years ago

TraceLogger: Cleanups.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: h4writer, Assigned: h4writer)

References

Details

Attachments

(11 files, 2 obsolete files)

1.50 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
20.33 KB, patch
till
: review+
Details | Diff | Splinter Review
4.72 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
3.33 KB, patch
till
: review+
Details | Diff | Splinter Review
13.43 KB, patch
till
: review+
Details | Diff | Splinter Review
4.70 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
4.40 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
59.95 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
4.38 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
716 bytes, patch
bbouvier
: review+
Details | Diff | Splinter Review
5.65 KB, patch
bhackett1024
: review+
Details | Diff | Splinter Review
Last bits to have a successful first integration of TraceLogger in Debugger
Blocks: 1065722
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).
Assignee: nobody → hv1989
Attachment #8508053 - Flags: review?(benj)
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.
Attachment #8508060 - Flags: review?(till)
Attachment #8508062 - Flags: review?(benj)
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).
Attachment #8508065 - Flags: review?(bbouvier)
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.
Attachment #8508078 - Flags: review?(benj)
Attachment #8508084 - Flags: review?(till)
Attachment #8508087 - Flags: review+
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 [..]"?
Attachment #8508060 - Flags: review?(till)
Comment on attachment 8508084 [details] [diff] [review]
Part 6: use js_ for new and delete

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

Oh, absolutely.
Attachment #8508084 - Flags: review?(till) → review+
Comment on attachment 8508085 [details] [diff] [review]
Part 7: Make --disable-trace-logging work again.

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

Nice
Attachment #8508085 - Flags: review?(till) → review+
(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
Attachment #8508062 - Flags: review?(benj)
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.
Attachment #8508065 - Flags: review?(bbouvier) → review+
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?
Attachment #8508060 - Flags: review+
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.
Attachment #8508053 - Flags: review?(benj) → review+
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.
Attachment #8508062 - Attachment is obsolete: true
Attachment #8509607 - Flags: review?(benj)
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;
Attachment #8508078 - Flags: review?(benj)
Attachment #8509607 - Flags: review?(benj) → review+
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.
Attachment #8508078 - Attachment is obsolete: true
Attachment #8512663 - Flags: review?(benj)
(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.
Attachment #8512663 - Flags: review?(benj) → review+
We hit this on try and it is a bit silly to not support quotes in filenames.
Attachment #8518032 - Flags: review?(benj)
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.
Attachment #8518032 - Flags: review?(benj) → review+
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.
Attachment #8526031 - Flags: review?(benj)
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?
Attachment #8526031 - Flags: review?(benj) → review+
(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.
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 → ---
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.
Attachment #8541689 - Flags: review?(bhackett1024) → review+
I pushed this follow-up in order to fix the static-analysis build bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c63cf613373
You need to log in before you can comment on or make changes to this bug.