1030389 - Instrument IonMonkey to Expose Optimization Information for a JIT Coach

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Vincent St-Amour [:vstamour]]

To make it possible to build a JIT coach, IonMonkey would need to report its optimization decisions, which the coach could then analyze.

The current plan is to have IonMonkey record its decisions in a table and to have the profiler sample that information for the scripts it observes on the stack.

Comment 1

[Vincent St-Amour [:vstamour]]

Attachment: 0001-Instrument-Ion-to-record-optimization-information.patch

First draft of the instrumentation. Records optimization attempts, successes and failures for getprop and setprop operations.

I don't have much C++ experience, so I'm especially interested in comments about the code, style, etc. In particular, the string manipulation code (especially in jsinfer.cpp) feels clumsy, but I can't think of a much better way to do it.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8446906 [details] [diff] [review]
0001-Instrument-Ion-to-record-optimization-information.patch

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

The current patch manipulates many parts of the code which are not all well known by everybody.
Also it seems that you have unrelated optimizations in your patch, is this only for testing purpose?

I will suggest to split your patch in multiple tiny patches.

::: js/src/jit/IonBuilder.cpp
@@ +8629,5 @@
> +    strcpy(str, prefix);
> +    if(types) {
> +        types->toString(str+strlen(prefix), len-strlen(prefix));
> +    }
> +    addOptInfo(str);

I am not completely sure how you interface works, as sometimes you provide an allocated string or a constant string.
I feel like this API might be leaking the computed strings.

Also I am not sure we want to allocate strings, for performance reasons.

::: js/src/jit/x86/MacroAssembler-x86.h
@@ +579,5 @@
>      void mulBy3(const Register &src, const Register &dest) {
>          lea(Operand(src, src, TimesTwo), dest);
>      }
> +    void mulBy5(const Register &src, const Register &dest) {
> +        lea(Operand(src, src, TimesFour), dest);

I don't think this has anything to do with this patch.

::: js/src/jsinfer.cpp
@@ +572,5 @@
> +    }    
> +}
> +
> +bool // true if printed everything, false if we had to truncate
> +TypeSet::toString(char *str, int32_t len)

This modification can be moved to its own patch.

Comment 3

[Vincent St-Amour [:vstamour]]

Nicolas,

First, thanks a lot for the review!


Re the purpose of this patch:

This is exploratory work, many things will need to be redesigned before this is ready to be merged. At the moment, the goal is to get a prototype working to see what we can do with that information, and whether this is the right information to expose for a coach. I'm putting this patch out there to give people a rough idea of the changes I'm making. Nothing should be considered final.

For example, that's why everything uses strings. As I mentioned in a comment, a more proper solution would use a better data format (e.g. enums to represent implementation strategies, optimization failures, etc., reusing some of the existing compiler data structures). We're using strings for now as quick and dirty first draft, to get a prototype working quickly.

Because this work spans multiple components, designing the right data format and API may be tricky (we probably don't want the profiler depending on Ion's internal data structures, for instance), so I think it would be better to wait until we really know this is the information we want to expose before spending time designing the interface.


Re splitting into multiple patches: Good idea, I'll do that.


Re allocated strings vs constant strings:

The allocated strings use the IonBuilder's allocator which (IIUC) frees everything after we're done with the IonBuilder. If that's the case, then I don't think anything should leak. Did I understand IonAllocPolicy right? I'll add a comment explaining that.

As for the performance impact of allocating strings, using better data structures (as I discuss above), which should solve the problem.


Re `mulBy5`:

That's now used by the macro-assembler. I added fields to SPS stack frames, which changes the size computations in `spsProfileEntryAddressSafe`. Are you saying it would be better to use a plain multiplication there, or that I should just put `mulBy5` in a separate patch?


I'll split the patch into smaller ones.

Thanks again!

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Vincent St-Amour [:vstamour] from comment #3)
> Re allocated strings vs constant strings:
> 
> The allocated strings use the IonBuilder's allocator which (IIUC) frees
> everything after we're done with the IonBuilder. If that's the case, then I
> don't think anything should leak. Did I understand IonAllocPolicy right?
> I'll add a comment explaining that.

In this case you are using the LifoAlloc of IonBuilder, but in other case you are using js_pod_malloc.  Both are bad for what you are trying to do.  As you mentioned the first one is bound to IonBuilder instance, which means that you allocate the strings as part of the lifo-alloc which is freed when IonBuilder is destroyed.  The way the SPS profiler works, you should avoid having non-static strings as in order to be fast the string are only converted when you ask for the profile AFAIU.  In the second case, you will just be leaking strings.

> As for the performance impact of allocating strings, using better data
> structures (as I discuss above), which should solve the problem.

ok.

> Re `mulBy5`:
> 
> That's now used by the macro-assembler. I added fields to SPS stack frames,
> which changes the size computations in `spsProfileEntryAddressSafe`. Are you
> saying it would be better to use a plain multiplication there, or that I
> should just put `mulBy5` in a separate patch?

I meant that this is a separate topic, and thus it deserve a separate patch ;)

Comment 5

[Vincent St-Amour [:vstamour]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Vincent St-Amour [:vstamour] from comment #3)
> > Re allocated strings vs constant strings:
> > 
> > The allocated strings use the IonBuilder's allocator which (IIUC) frees
> > everything after we're done with the IonBuilder. If that's the case, then I
> > don't think anything should leak. Did I understand IonAllocPolicy right?
> > I'll add a comment explaining that.
> 
> In this case you are using the LifoAlloc of IonBuilder, but in other case
> you are using js_pod_malloc.  Both are bad for what you are trying to do.

I think I've been careful to only use js_pod_malloc for things that I
know when to free (and do free) . For example, strings stored in the
table inside the SPSProfiler object. Those are freed when the
corresponding script is finalized.

I'll double check to make sure.

> As you mentioned the first one is bound to IonBuilder instance, which means
> that you allocate the strings as part of the lifo-alloc which is freed when
> IonBuilder is destroyed.

Right, but by that time, the strings have been copied to the SPSProfiler
object, so we should be good, at least as correctness is concerned.

> The way the SPS profiler works, you should avoid having non-static
> strings as in order to be fast the string are only converted when you
> ask for the profile AFAIU.  In the second case, you will just be
> leaking strings.

Right, I think that's what's done with other generated strings used by
the profiler. Since we shouldn't be using strings in the long run, I
won't worry about that for now.

However, we'll probably still need some amount of non-static information
in the end. TI typesets come to mind. In that case, though, I hope to
eventually record constructor locations (which is not completely static,
but is at least per-script) instead. Other types (e.g. string) are
represented as static strings, so we should be good. Things like number
of shapes observed are dynamic but they're integers, so that shouldn't
be a problem.

> > Re `mulBy5`:
> > 
> > That's now used by the macro-assembler. I added fields to SPS stack frames,
> > which changes the size computations in `spsProfileEntryAddressSafe`. Are you
> > saying it would be better to use a plain multiplication there, or that I
> > should just put `mulBy5` in a separate patch?
> 
> I meant that this is a separate topic, and thus it deserve a separate patch
> ;)

Yep, I'm splitting it off into a separate patch.

I looked into trying to use regular multiplication instead, but there
doesn't seem to be a platform-independent multiplication macro-instruction.
Adding `mulBy5` sounds more desirable than making `spsProfileEntryAddressSafe`
architecture-specific, so I'll stick to that.

Comment 6

[Vincent St-Amour [:vstamour]]

Attachment: 0001-Add-a-toString-function-to-typesets.patch

Comment 7

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Instrument-Ion-to-record-optimization-information.patch

Comment 8

[Vincent St-Amour [:vstamour]]

Attachment: 0003-Merge-inlined-script-s-optimization-info-with-their-.patch

Comment 9

[Vincent St-Amour [:vstamour]]

Attachment: 0004-Copy-optimization-info-over-to-SPSProfiler-objects.patch

Comment 10

[Vincent St-Amour [:vstamour]]

Attachment: 0005-Add-a-mulBy5-macro-instruction.patch

Comment 11

[Vincent St-Amour [:vstamour]]

Attachment: 0006-Fix-typo-in-comment-in-macro-assembler.patch

Comment 12

[Vincent St-Amour [:vstamour]]

Attachment: 0007-Push-pointer-to-optimization-information-on-the-SPS-.patch

Comment 13

[Vincent St-Amour [:vstamour]]

Attachment: 0008-Sample-optimization-information-when-profiling.patch

Comment 14

[Vincent St-Amour [:vstamour]]

Split into smaller patches.

Comment 15

[Vincent St-Amour [:vstamour]]

Attachment: 0004-Add-a-mulBy5-macro-instruction.patch

Comment 16

[Vincent St-Amour [:vstamour]]

Attachment: 0005-Fix-typo-in-comment-in-macro-assembler.patch

Comment 17

[Vincent St-Amour [:vstamour]]

Attachment: 0006-Commit-explaining-allocation-behavior-of-optimizatio.patch

Comment 18

[Vincent St-Amour [:vstamour]]

Attachment: 0007-Have-counter-that-identifies-compiles.patch

Comment 19

[Vincent St-Amour [:vstamour]]

Attachment: 0008-Store-compilation-IDs-in-SPS-profiler-objects.patch

Comment 20

[Vincent St-Amour [:vstamour]]

Attachment: 0009-Push-compile-IDs-to-the-SPS-stack.patch

Comment 21

[Vincent St-Amour [:vstamour]]

Attachment: 0010-Sample-compile-IDs-when-profiling.patch

Comment 22

[Vincent St-Amour [:vstamour]]

Trying a new, simpler implementation strategy.

Instead of carrying arrays of strings all the way to the sampler, log optimization info once per compile as a profiling event. Then, to track which samples correspond to which compiles, have samples contain an int that identifies the compile that produced the code. These integers are now the only information that needs to be carried from Ion to the sampler.

With this approach, we avoid any need for the sampler to depend on compiler internals to interpret optimization info. I was previously working around that by passing strings to the sampler, but this new strategy avoids the problem altogether by not having the sampler deal with optimization info at all.

This strategy could be simplified further. Instead of storing compile IDs in a table in the SPSProfiler, I think they could be stored in IonScripts or anything else available from spsPushFrame. I'll look into that.

Comment 23

[Nicolas B. Pierron [:nbp]]

(In reply to Vincent St-Amour [:vstamour] from comment #22)
> Trying a new, simpler implementation strategy.
> 
> […]

Overall the idea sounds good :)

> This strategy could be simplified further. Instead of storing compile IDs in
> a table in the SPSProfiler, I think they could be stored in IonScripts or
> anything else available from spsPushFrame. I'll look into that.

Take care, after an invalidation, IonScript are only are ref-counted, such we garage collect them when there is no more instance on the Stack.  You might have to cheat the same way, or to copy the information out of IonScripts when they are invalidated.

Comment 24

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Instrument-Ion-to-record-optimization-information.patch

Comment 25

[Vincent St-Amour [:vstamour]]

Attachment: 0007-Have-counter-that-identifies-compiles.patch

Comment 26

[Vincent St-Amour [:vstamour]]

Attachment: 0008-Emit-profiler-events-containing-optimization-informa.patch

Comment 27

[Vincent St-Amour [:vstamour]]

That turned out to be even simpler than I thought.

We don't need to add anything to the IonScript to avoid the need for the table. The code generator can just look up the compile ID on its MIRGenerator.

Comment 28

[Vincent St-Amour [:vstamour]]

Attachment: 0009-Push-compile-IDs-to-the-SPS-stack.patch

Comment 29

[Nicolas B. Pierron [:nbp]]

(In reply to Vincent St-Amour [:vstamour] from comment #22)
> Instead of carrying arrays of strings all the way to the sampler, log
> optimization info once per compile as a profiling event. Then, to track
> which samples correspond to which compiles, have samples contain an int that
> identifies the compile that produced the code. These integers are now the
> only information that needs to be carried from Ion to the sampler.

This sounds strangely familiar.  For your information this is exactly what we currently do for the Type constraints, except that the ID might not be valid after a GC.  Have a look at RecompileInfo [1], which is used to identify one compilation output, to ensure that we do not invalidate the newly recompiled code which is attached on the JSScript.  This is also use to distinguish between PJS and sequential execution.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCode.h?from=IonScript#283

Comment 30

[Vincent St-Amour [:vstamour]]

Interesting. I didn't know about RecompileInfo.

It looks like RecompileInfo itself is only available after linking, which is too late given my current implementation. I'd need a compilation ID both right after Ion building (when emitting profile events, though that could could probably be done during/after linking instead) and during code generation (to push it on the SPS frame).

However, it does look like we may be able to reuse what RecompileInfo uses to identify compiles (the length of a TypeZone's compilerOutputs vector). I'll look into it.

Thanks for the pointer!

Comment 31

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Instrument-Ion-to-record-optimization-information.patch

Comment 32

[Vincent St-Amour [:vstamour]]

Attachment: 0006-Comment-explaining-allocation-behavior-of-optimizati.patch

Comment 33

[Vincent St-Amour [:vstamour]]

Attachment: 0011-Make-object-type-printing-available-outside-of-debug.patch

Comment 34

[Vincent St-Amour [:vstamour]]

Attachment: 0012-Allow-printing-constructor-when-printing-object-type.patch

Comment 35

[Vincent St-Amour [:vstamour]]

Updated to log constructor location when logging object types, in addition to the address of the object type data structure. This makes it easier to pinpoint the relevant code in the coach's reports.

The updated prototype coach is available here: https://github.com/stamourv/jit-coach/

Comment 36

[Vincent St-Amour [:vstamour]]

Attachment: 0002-Instrument-Ion-to-record-optimization-information.patch

Comment 37

[Vincent St-Amour [:vstamour]]

Attachment: 0011-Make-object-type-printing-available-outside-of-debug.patch

Comment 38

[Vincent St-Amour [:vstamour]]

Attachment: 0012-Allow-printing-constructor-when-printing-object-type.patch

Comment 39

[Vincent St-Amour [:vstamour]]

Minor fixes.

Comment 40

[Vincent St-Amour [:vstamour]]

Attachment: 0013-Allow-logging-optimization-attempts-with-no-associat.patch

Comment 41

[Vincent St-Amour [:vstamour]]

Attachment: 0014-Add-helper-to-log-MIR-types.patch

Comment 42

[Vincent St-Amour [:vstamour]]

Attachment: 0015-Add-optimization-logging-for-getelem.patch

Comment 43

[Vincent St-Amour [:vstamour]]

Attachment: 0016-Add-optimization-logging-for-setelem.patch

Comment 44

[Vincent St-Amour [:vstamour]]

Attachment: 0017-Have-JSScript-ids-be-meaningful-outside-of-debug-mod.patch

Comment 45

[Vincent St-Amour [:vstamour]]

Added instrumentation for getelem and setelem.

Comment 46

[Vincent St-Amour [:vstamour]]

Attachment: 0015-Add-optimization-logging-for-getelem.patch

Comment 47

[Vincent St-Amour [:vstamour]]

Attachment: 0016-Add-optimization-logging-for-setelem.patch

Comment 48

[Shu-yu Guo [:shu]]

Taking over hacking on this now that Vincent's internship is over.

Comment 49

[Shu-yu Guo [:shu]]

Attachment: Part 1: Optimization strategy tracking infrastructure.

Pretty big patch. Let me know if we should do a code walk.

The encodings are trained on Octane and automatically generated by a Python
script, attached below. The script uses an eager coverage algorithm: it tries
to cover as much of the training data as possible for the smaller encodings
before trying to determine bigger encodings.

Comment 50

[Shu-yu Guo [:shu]]

Attachment: Part 2: Track IonBuilder::jsop_getprop optimization.

Comment 51

[Shu-yu Guo [:shu]]

Attachment: compute_encs.py

Script used to process training data spewed from IONFLAGS=trackopts to generate locally optimal (most compact) encodings.

Comment 52

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #49)
> Created attachment 8514649 [details] [diff] [review]
> Part 1: Optimization strategy tracking infrastructure.

I think the frequency fields should be named occurences for the moment, as until we got a real profiler in Baseline, as Wu Wei implemented previously, we do not have any hints on the frequency.

Also, this is not because one thing is not optimized at all that this is necessary the worse part.  For example, if a branch is never taken, we might report anything from within this branch, even if we have 50 identical occurrences within this non-taken branch, and we might forget about the only single occurrence in the taken branch.

Comment 53

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #52)
> (In reply to Shu-yu Guo [:shu] from comment #49)
> > Created attachment 8514649 [details] [diff] [review]
> > Part 1: Optimization strategy tracking infrastructure.
> 
> I think the frequency fields should be named occurences for the moment, as
> until we got a real profiler in Baseline, as Wu Wei implemented previously,
> we do not have any hints on the frequency.
> 

'Frequency' here refers to the frequency with which a certain sequence of optimization attempts occurs in the compilation. This frequency isn't used to report how often an optimization will actually be hit, it's used to order the index for compacting, so optimization attempts vectors that are repeated more often get a lower index, and thus less bits.

> Also, this is not because one thing is not optimized at all that this is
> necessary the worse part.  For example, if a branch is never taken, we might
> report anything from within this branch, even if we have 50 identical
> occurrences within this non-taken branch, and we might forget about the only
> single occurrence in the taken branch.

As we discussed on IRC, the ultimate API is to be map native code ptrs -> optimization attempts from profiler samples. This infrastructure records all optimization attempts made into a table to be consulted later by the profiler.

Comment 54

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Optimization strategy tracking infrastructure.

Un-bitrotted.

Comment 55

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_getprop optimization.

Comment 56

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Track type info used by optimizations.

Comment 57

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Track Ion aborts.

Comment 58

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_setprop optimization.

Comment 59

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track calls.

Comment 60

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Optimization strategy tracking infrastructure.

Comment 61

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_getprop optimization.

Comment 62

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_setprop optimization.

Comment 63

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track calls.

Comment 64

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_getelem optimizations.

Comment 65

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Comment 66

[Kannan Vijayan [:djvj]]

Comment on attachment 8540460 [details] [diff] [review]
Infrastructure: Track Ion aborts.

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

Overall looks good.  I want to take a look at it again after you rebase to post-profiler-patches landing.  Should be a quick r+ after that.

::: js/src/jit/Ion.cpp
@@ +1873,5 @@
> +static void
> +TrackIonAbort(JSContext *cx, JSScript *script, jsbytecode *pc, const char *message)
> +{
> +    if (!cx->runtime()->jitRuntime()->isOptimizationTrackingEnabled(cx->runtime()))
> +        return;

If optimization tracking is not enabled.. then |builder->hasActionableAbort()| should never return true, and thus |TrackIonAbort| should never be called.  Should this be an ASSERT instead?

@@ +1878,5 @@
> +
> +    // Only bother tracking aborts of functions we're attempting to
> +    // Ion-compile after successfully running in Baseline.
> +    if (!script->hasBaselineScript())
> +        return;

This too seems like an ASSERT to me.  We should never attempt to Ion-compile a script which has not already been baseline-compiled.

@@ +2314,5 @@
>      if (state.isInvoke()) {
>          InvokeState &invoke = *state.asInvoke();
>  
>          if (TooManyActualArguments(invoke.args().length())) {
> +            TrackAndSpewIonAbort(cx, script, "too many actual args");

Nit: arguments.

These are going to show up in frontend.  Good to use full words instead of abbrevs.

@@ +2320,5 @@
>              return Method_CantCompile;
>          }
>  
>          if (TooManyFormalArguments(invoke.args().callee().as<JSFunction>().nargs())) {
> +            TrackAndSpewIonAbort(cx, script, "too many args");

Nit: arguments.

::: js/src/jit/IonBuilder.h
@@ +899,5 @@
> +    // Some aborts are actionable (e.g., using an unsupported bytecode). When
> +    // optimization tracking is enabled, the location and message of the abort
> +    // are recorded here so they may be propagated to the script's
> +    // corresponding JitcodeGlobalEntry::BaselineEntry.
> +    JSScript *actionableAbortScript_;

I wonder if this wouldn't be better as an InlineScriptTree*, which will identify the exact inlining path that led to the abort.  Just something to consider.

::: js/src/jit/JitCompartment.h
@@ +426,5 @@
>  #endif // DEBUG
>      }
> +
> +    bool isOptimizationTrackingEnabled(JSRuntime *rt) {
> +        return isNativeToBytecodeMapEnabled(rt);

This will need to be rebaesd and changed to use to isProfilerInstrumentationEnabled(), after my profiling patches landed.  Apologies.

Comment 67

[Shu-yu Guo [:shu]]

(In reply to Kannan Vijayan [:djvj] from comment #66)
> Comment on attachment 8540460 [details] [diff] [review]
> Infrastructure: Track Ion aborts.
> 
> Review of attachment 8540460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall looks good.  I want to take a look at it again after you rebase to
> post-profiler-patches landing.  Should be a quick r+ after that.
> 
> ::: js/src/jit/Ion.cpp
> @@ +1873,5 @@
> > +static void
> > +TrackIonAbort(JSContext *cx, JSScript *script, jsbytecode *pc, const char *message)
> > +{
> > +    if (!cx->runtime()->jitRuntime()->isOptimizationTrackingEnabled(cx->runtime()))
> > +        return;
> 
> If optimization tracking is not enabled.. then
> |builder->hasActionableAbort()| should never return true, and thus
> |TrackIonAbort| should never be called.  Should this be an ASSERT instead?
> 

Actually no, because a bunch of the CheckX functions like CheckFrame may be called when this condition is false. I could duplicate code and have different versions of CheckFrame or something, but I decided this was cleaner.

> @@ +1878,5 @@
> > +
> > +    // Only bother tracking aborts of functions we're attempting to
> > +    // Ion-compile after successfully running in Baseline.
> > +    if (!script->hasBaselineScript())
> > +        return;
> 
> This too seems like an ASSERT to me.  We should never attempt to Ion-compile
> a script which has not already been baseline-compiled.

Same reason as above. CheckFrame is called from jit::CompileFunctionForBaseline.

> 
> @@ +2314,5 @@
> >      if (state.isInvoke()) {
> >          InvokeState &invoke = *state.asInvoke();
> >  
> >          if (TooManyActualArguments(invoke.args().length())) {
> > +            TrackAndSpewIonAbort(cx, script, "too many actual args");
> 
> Nit: arguments.
> 
> These are going to show up in frontend.  Good to use full words instead of
> abbrevs.
> 
> @@ +2320,5 @@
> >              return Method_CantCompile;
> >          }
> >  
> >          if (TooManyFormalArguments(invoke.args().callee().as<JSFunction>().nargs())) {
> > +            TrackAndSpewIonAbort(cx, script, "too many args");
> 
> Nit: arguments.
> 

Will fix.

> ::: js/src/jit/IonBuilder.h
> @@ +899,5 @@
> > +    // Some aborts are actionable (e.g., using an unsupported bytecode). When
> > +    // optimization tracking is enabled, the location and message of the abort
> > +    // are recorded here so they may be propagated to the script's
> > +    // corresponding JitcodeGlobalEntry::BaselineEntry.
> > +    JSScript *actionableAbortScript_;
> 
> I wonder if this wouldn't be better as an InlineScriptTree*, which will
> identify the exact inlining path that led to the abort.  Just something to
> consider.
> 

I originally had an InlineScriptTree *, but it didn't add any more information. The exact inlining path isn't needed for this case since this is tracking catastrophic aborts, which is a per-script thing and not a per-callsite thing.

> ::: js/src/jit/JitCompartment.h
> @@ +426,5 @@
> >  #endif // DEBUG
> >      }
> > +
> > +    bool isOptimizationTrackingEnabled(JSRuntime *rt) {
> > +        return isNativeToBytecodeMapEnabled(rt);
> 
> This will need to be rebaesd and changed to use to
> isProfilerInstrumentationEnabled(), after my profiling patches landed. 
> Apologies.

Comment 68

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Optimization strategy tracking infrastructure.

Rebased.

Comment 69

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Track Ion aborts.

Rebased and fixed 'args'.

Comment 70

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_getprop optimization.

Rebased.

Comment 71

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_setprop optimization.

Rebased.

Comment 72

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track calls.

Rebased.

Comment 73

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_getelem optimizations.

Rebased.

Comment 74

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Rebased.

Comment 75

[Kannan Vijayan [:djvj]]

Comment on attachment 8552150 [details] [diff] [review]
Infrastructure: Optimization strategy tracking infrastructure.

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

General note: In CompileInfo, I think it would be better to have a |hasOptimizationInfo| method that checks nullness of the optimizations_ field.  Then rename the |optimizations()| method to |maybeOptimizations()|, and then add a proper |optimizatinos()| method that does ASSERT(hasOptimizatinoInfo()).

::: js/src/jit/JitcodeMap.h
@@ +110,5 @@
> +        // optsRegionTable_ points to the table within the compact
> +        // optimizations map indexing all regions that have tracked
> +        // optimization attempts. optsTypesTable_ is the tracked typed info
> +        // associated with the attempts vectors; it is the same length as the
> +        // attempts table. optsAttemptsTable_ is the table indexing those

Wait, is this true (optsTypesTable_.length == optsAttemptsTable_.length) ?  I thought the types table unique-ified the types sets for memory savings..

@@ +155,5 @@
>              BaseEntry::init(Ion, nativeStartAddr, nativeEndAddr);
>              regionTable_ = regionTable;
>              scriptList_ = scriptList;
> +            optsRegionTable_ = nullptr;
> +            optsAttemptsTable_ = nullptr;

also: optsTypesTable_ = nullptr;
      oiptsAllTypes_ = nullptr;

::: js/src/jit/OptimizationTracking.cpp
@@ +156,5 @@
> +      default:
> +        MOZ_CRASH("bad tracked type info kind");
> +    }
> +}
> +#endif

Nit: #endif is faraway from #ifdef.  |#endif // DEBUG| would be clearer.

@@ +217,5 @@
> +bool
> +TrackedTypeInfo::operator !=(const TrackedTypeInfo &other) const
> +{
> +    return kind_ != other.kind_ || mirType_ != other.mirType_ ||
> +           !VectorContentsMatch(&types_, &other.types_);

Why not !(*this == other) ?

@@ +358,5 @@
> +    return true;
> +}
> +
> +uint8_t
> +UniqueTrackedOptimizations::index(const TrackedOptimizations *optimizations) const

Nit: indexOf would be a better name.

@@ +370,5 @@
> +    MOZ_ASSERT(p->value().index != UINT8_MAX);
> +    return p->value().index;
> +}
> +
> +// Assigns each unique type tracked an index; outputs a compact list.

Nite: unique tracked type

@@ +472,5 @@
> +bool
> +JitcodeGlobalEntry::IonEntry::optimizationAttemptsAtAddr(void *ptr,
> +                                                         Maybe<AttemptsVector> &attempts)
> +{
> +    MOZ_ASSERT(containsPointer(ptr));

Should this also have MOZ_ASSERT(!attempts) at the top?

@@ +820,5 @@
> +    // Record the start of the table to compute reverse offsets for entries.
> +    uint32_t tableOffset = writer.length();
> +
> +    // Write how many bytes were padded and numEntries.
> +    writer.writeNativeEndianUint32_t(padding);

I'm not sure why you need to write out the number of padding bytes.  I think in the native=>bytecode mapping encoding, the last entry in the table is a dummy entry pointing to the offset of the end.  It's sort of the same idea, but it seems more regular.

@@ +827,5 @@
> +    // Write entry offset table.
> +    for (size_t i = 0; i < offsets.length(); i++) {
> +        JitSpew(JitSpew_OptimizationTracking, "   Entry %u reverse offset %u",
> +                i, tableOffset - offsets[i]);
> +        writer.writeNativeEndianUint32_t(tableOffset - padding - offsets[i]);

Shouldn't this be writer.writeNativeEndianUint32_t(tableOFfset - offsets[i])?

The padding shouldn't make a difference with respect to the reverse distance from tableOffset to offsets[i].

::: js/src/jit/OptimizationTracking.h
@@ +432,5 @@
> +
> +    static const uint32_t ENC4_INDEX_MAX = 0xff;
> +    static const uint32_t ENC4_INDEX_SHIFT = 3;
> +
> +    static bool IsDeltaEncodeable(uint32_t startDelta, uint32_t length) {

So are indexes guaranteed to be <256?  What do we do if we generate more than that?

::: js/src/vm/ArrayBufferObject.cpp
@@ +1550,5 @@
>          return nullptr;
>  
>      RootedId byteLengthId(cx, NameToId(cx->names().byteLength));
>      unsigned attrs = JSPROP_SHARED | JSPROP_GETTER;
> +    JSObject *getter = NewFunction(cx, js::NullPtr(), ArrayBufferObject::byteLengthGetter, 0,

Doe this belong in this patch?  This seems like fixups for namespacing changes that are tangential..

Comment 76

[Shu-yu Guo [:shu]]

I'm not uploading a new patch, since the fixes were nits. Hopefully I clarified the questions you had, which didn't turn out to be bugs.

(In reply to Kannan Vijayan [:djvj] from comment #75)
> Comment on attachment 8552150 [details] [diff] [review]
> Infrastructure: Optimization strategy tracking infrastructure.
> 
> Review of attachment 8552150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> General note: In CompileInfo, I think it would be better to have a
> |hasOptimizationInfo| method that checks nullness of the optimizations_
> field.  Then rename the |optimizations()| method to |maybeOptimizations()|,
> and then add a proper |optimizatinos()| method that does
> ASSERT(hasOptimizatinoInfo()).
> 

Will fix.

> ::: js/src/jit/JitcodeMap.h
> @@ +110,5 @@
> > +        // optsRegionTable_ points to the table within the compact
> > +        // optimizations map indexing all regions that have tracked
> > +        // optimization attempts. optsTypesTable_ is the tracked typed info
> > +        // associated with the attempts vectors; it is the same length as the
> > +        // attempts table. optsAttemptsTable_ is the table indexing those
> 
> Wait, is this true (optsTypesTable_.length == optsAttemptsTable_.length) ? 
> I thought the types table unique-ified the types sets for memory savings..
> 

So, this is a bit confusing. The vector that is deduplicated includes the type info. So, [type1, type2, ..., typeN, attempt1, attempt2, ..., attemptM]. This vector, for ease of encoding and decoding, is then split into 2 vectors, [type1, type2, ..., typeN], and [attempt1, attempt2, ..., attemptM]. That's why these 2 tables have to be the same.

Types undergo further deduplication, such that the "typeN" above is stored as an index into the allTypes vector, which is the deduplicated vector of all tracked types.

> @@ +155,5 @@
> >              BaseEntry::init(Ion, nativeStartAddr, nativeEndAddr);
> >              regionTable_ = regionTable;
> >              scriptList_ = scriptList;
> > +            optsRegionTable_ = nullptr;
> > +            optsAttemptsTable_ = nullptr;
> 
> also: optsTypesTable_ = nullptr;
>       oiptsAllTypes_ = nullptr;
> 
> ::: js/src/jit/OptimizationTracking.cpp
> @@ +156,5 @@
> > +      default:
> > +        MOZ_CRASH("bad tracked type info kind");
> > +    }
> > +}
> > +#endif
> 
> Nit: #endif is faraway from #ifdef.  |#endif // DEBUG| would be clearer.
> 
> @@ +217,5 @@
> > +bool
> > +TrackedTypeInfo::operator !=(const TrackedTypeInfo &other) const
> > +{
> > +    return kind_ != other.kind_ || mirType_ != other.mirType_ ||
> > +           !VectorContentsMatch(&types_, &other.types_);
> 
> Why not !(*this == other) ?
> 

Good point. :)

> @@ +358,5 @@
> > +    return true;
> > +}
> > +
> > +uint8_t
> > +UniqueTrackedOptimizations::index(const TrackedOptimizations *optimizations) const
> 
> Nit: indexOf would be a better name.
> 
> @@ +370,5 @@
> > +    MOZ_ASSERT(p->value().index != UINT8_MAX);
> > +    return p->value().index;
> > +}
> > +
> > +// Assigns each unique type tracked an index; outputs a compact list.
> 
> Nite: unique tracked type
> 
> @@ +472,5 @@
> > +bool
> > +JitcodeGlobalEntry::IonEntry::optimizationAttemptsAtAddr(void *ptr,
> > +                                                         Maybe<AttemptsVector> &attempts)
> > +{
> > +    MOZ_ASSERT(containsPointer(ptr));
> 
> Should this also have MOZ_ASSERT(!attempts) at the top?
> 
> @@ +820,5 @@
> > +    // Record the start of the table to compute reverse offsets for entries.
> > +    uint32_t tableOffset = writer.length();
> > +
> > +    // Write how many bytes were padded and numEntries.
> > +    writer.writeNativeEndianUint32_t(padding);
> 
> I'm not sure why you need to write out the number of padding bytes.  I think
> in the native=>bytecode mapping encoding, the last entry in the table is a
> dummy entry pointing to the offset of the end.  It's sort of the same idea,
> but it seems more regular.
> 
> @@ +827,5 @@
> > +    // Write entry offset table.
> > +    for (size_t i = 0; i < offsets.length(); i++) {
> > +        JitSpew(JitSpew_OptimizationTracking, "   Entry %u reverse offset %u",
> > +                i, tableOffset - offsets[i]);
> > +        writer.writeNativeEndianUint32_t(tableOffset - padding - offsets[i]);
> 
> Shouldn't this be writer.writeNativeEndianUint32_t(tableOFfset - offsets[i])?
> 
> The padding shouldn't make a difference with respect to the reverse distance
> from tableOffset to offsets[i].
> 

So the padding is there because of how I track the offsets table. I write padded bytes to avoid having special logic to deal with the last region, which may or may not have trailing padding. If I record the padding, I can be sure that each region, when reading from the CompactBufferReader, only refers to the payload and nothing else.

So how I get to the starting offset of a region is:

  entryStart = payloadEnd - entryReverseOffset         (1)

where

  payloadEnd = tablePtr - padding                      (2)

So, when recording the reverse offsets, I compute

  entryReverseOffset = tablePtr - padding - entryStart (3)

pf. substituting (2) and (3) into (1)

  entryStart = tablePtr - padding - (tablePtr - padding - entryStart)
  entryStart = tablePtr - padding - tablePtr + padding + entryStart
  entryStart = tablePtr - tablePtr + padding - padding + entryStart
  entryStart = entryStart

> ::: js/src/jit/OptimizationTracking.h
> @@ +432,5 @@
> > +
> > +    static const uint32_t ENC4_INDEX_MAX = 0xff;
> > +    static const uint32_t ENC4_INDEX_SHIFT = 3;
> > +
> > +    static bool IsDeltaEncodeable(uint32_t startDelta, uint32_t length) {
> 
> So are indexes guaranteed to be <256?  What do we do if we generate more
> than that?
> 

We discussed this in person. We can expand this later.

> ::: js/src/vm/ArrayBufferObject.cpp
> @@ +1550,5 @@
> >          return nullptr;
> >  
> >      RootedId byteLengthId(cx, NameToId(cx->names().byteLength));
> >      unsigned attrs = JSPROP_SHARED | JSPROP_GETTER;
> > +    JSObject *getter = NewFunction(cx, js::NullPtr(), ArrayBufferObject::byteLengthGetter, 0,
> 
> Doe this belong in this patch?  This seems like fixups for namespacing
> changes that are tangential..

It was to make it to compile at all. There's an ambiguity between JS::NullPtr and js::NullPtr such that everytime we shift the unified source boundaries there's a chance that things break.

Comment 77

[Kannan Vijayan [:djvj]]

Comment on attachment 8552150 [details] [diff] [review]
Infrastructure: Optimization strategy tracking infrastructure.

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

Forgot to cancel review for this.  Re-r? me after updates.

Comment 78

[Shu-yu Guo [:shu]]

Attachment: Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Addressed comments. No real changes though, per comment 76.

Comment 79

[Kannan Vijayan [:djvj]]

Comment on attachment 8552153 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_getprop optimization.

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

::: js/src/jit/IonBuilder.cpp
@@ +9785,5 @@
>  IonBuilder::getPropTryDefiniteSlot(bool *emitted, MDefinition *obj, PropertyName *name,
>                                     BarrierKind barrier, types::TemporaryTypeSet *types)
>  {
>      MOZ_ASSERT(*emitted == false);
> +

Nit: whitespace on blank line.

::: js/src/jit/OptimizationTracking.cpp
@@ +137,5 @@
>        case OptimizationAttempt::Outcome_InDictionaryMode:
>          return "object in dictionary mode";
> +
> +      case OptimizationAttempt::Outcome_CantInlineGeneric:
> +        return "can't inline";

If you convert the OptimizationAttempt enum to the CPP macro-iterator style, this whole method becomes a 3-liner.

::: js/src/jit/OptimizationTracking.h
@@ +76,2 @@
>          Outcome_Monomorphic,
>          Outcome_Polymorphic,

Can you convert all of these to the CPP macro-iterator style we use for MIR opcodes and stuff?  It seems pretty apparent that they're going to grow large soon.

Comment 80

[Kannan Vijayan [:djvj]]

Comment on attachment 8552155 [details] [diff] [review]
Instrumentation: Track calls.

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

::: js/src/jit/MCallOptimize.cpp
@@ +1427,5 @@
>          return InliningStatus_NotInlined;
>  
>      JSString *str = strval->toString();
> +    if (!str->isLinear()) {
> +        trackOptimizationOutcome(OptimizationAttempt::Outcome_CantInlineGeneric);

Maybe have an Outcome_NonSimpleString ?

@@ +1434,4 @@
>  
>      int32_t idx = idxval->toInt32();
> +    if (idx < 0 || (uint32_t(idx) >= str->length())) {
> +        trackOptimizationOutcome(OptimizationAttempt::Outcome_CantInlineGeneric);

Maybe Outcome_IndexOutOfRange?

Comment 81

[Shu-yu Guo [:shu]]

Comment on attachment 8554746 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Oops, accidentally uploaded 2 copies of this patch.

Comment 82

[Shu-yu Guo [:shu]]

Attachment: Infrastructure: Optimization strategy tracking infrastructure.

Made enums into enum classes and hoisted them out of
OptimizationAttempt/TrackedTypeInfo so they're a bit easier to type.

Comment 83

[Shu-yu Guo [:shu]]

(In reply to Kannan Vijayan [:djvj] from comment #80)
> Comment on attachment 8552155 [details] [diff] [review]
> Instrumentation: Track calls.
> 
> Review of attachment 8552155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +1427,5 @@
> >          return InliningStatus_NotInlined;
> >  
> >      JSString *str = strval->toString();
> > +    if (!str->isLinear()) {
> > +        trackOptimizationOutcome(OptimizationAttempt::Outcome_CantInlineGeneric);
> 
> Maybe have an Outcome_NonSimpleString ?

I'm not sure how actionable this can be.

> 
> @@ +1434,4 @@
> >  
> >      int32_t idx = idxval->toInt32();
> > +    if (idx < 0 || (uint32_t(idx) >= str->length())) {
> > +        trackOptimizationOutcome(OptimizationAttempt::Outcome_CantInlineGeneric);
> 
> Maybe Outcome_IndexOutOfRange?

Yeah, that's reasonable.

Comment 84

[Shu-yu Guo [:shu]]

Attachment: Dear fuzzfriends, please fuzz. Thanks!

Fuzzing rollup.

Bug 1030389 - Infrastructure: Optimization strategy tracking infrastructure.

No bug - Fix namespace error in jsgc.cpp due to shifted unified source boundaries. (r=me)

 Bug 1030389 - Infrastructure: Track Ion aborts. (r=djvj)

Bug 1030389 - Instrumentation: Track IonBuilder::jsop_getprop optimization.

Bug 1030389 - Instrumentation: Track IonBuilder::jsop_setprop optimization.

Bug 1030389 - Instrumentation: Track calls.

Bug 1030389 - Instrumentation: Track IonBuilder::jsop_getelem optimizations.

Bug 1030389 - Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Comment 85

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8554903 [details] [diff] [review]
Dear fuzzfriends, please fuzz. Thanks!

I couldn't apply this cleanly to m-c rev 38e4719e71af. Please rebase.

Comment 86

[Shu-yu Guo [:shu]]

Attachment: Rollup for fuzzing.

Rebased.

Comment 87

[Shu-yu Guo [:shu]]

Gary, to exercise the code in the patch SPS needs to be on. Can you prepend "enableSPSProfiling();" before all generated code?

Comment 88

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Shu-yu Guo [:shu] from comment #87)
> Gary, to exercise the code in the patch SPS needs to be on. Can you prepend
> "enableSPSProfiling();" before all generated code?

jsfunfuzz already tests enableSPSProfiling(), but I've boosted the probability of generating enableSPSProfiling() just for testing this patch.

Comment 89

[Kannan Vijayan [:djvj]]

Comment on attachment 8554895 [details] [diff] [review]
Infrastructure: Optimization strategy tracking infrastructure.

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

I just checked for comments being addressed, assumed remaining code was left the same.  Looks good.  Let's get the party started.

Comment 90

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8555107 [details] [diff] [review]
Rollup for fuzzing.

This did not immediately blow up jsfunfuzz after an overnight run.

Comment 91

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98498bec2d96
https://hg.mozilla.org/mozilla-central/rev/c75507637c20
https://hg.mozilla.org/mozilla-central/rev/98640174ff15
https://hg.mozilla.org/mozilla-central/rev/4951b75ba08f
https://hg.mozilla.org/mozilla-central/rev/0a6a3e804c3c
https://hg.mozilla.org/mozilla-central/rev/53eb207d6597
https://hg.mozilla.org/mozilla-central/rev/f3a78ec34fc1
https://hg.mozilla.org/mozilla-central/rev/08786e955d26