Instrument IonMonkey to Expose Optimization Information for a JIT Coach

RESOLVED FIXED in mozilla38

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: vstamour, Assigned: shu)

Tracking

(Blocks: 1 bug)

Trunk
mozilla38
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(26 attachments, 34 obsolete attachments)

4.16 KB, patch
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
2.78 KB, patch
Details | Diff | Splinter Review
1.25 KB, patch
Details | Diff | Splinter Review
5.01 KB, patch
Details | Diff | Splinter Review
4.01 KB, patch
Details | Diff | Splinter Review
2.62 KB, patch
Details | Diff | Splinter Review
9.78 KB, patch
Details | Diff | Splinter Review
1.07 KB, patch
Details | Diff | Splinter Review
22.66 KB, patch
Details | Diff | Splinter Review
4.28 KB, patch
Details | Diff | Splinter Review
10.27 KB, patch
Details | Diff | Splinter Review
3.07 KB, patch
Details | Diff | Splinter Review
2.21 KB, patch
Details | Diff | Splinter Review
2.33 KB, patch
Details | Diff | Splinter Review
9.67 KB, patch
Details | Diff | Splinter Review
5.68 KB, patch
Details | Diff | Splinter Review
4.98 KB, text/x-python
Details
17.59 KB, patch
djvj
: review+
Details | Diff | Splinter Review
37.48 KB, patch
djvj
: review+
Details | Diff | Splinter Review
18.38 KB, patch
djvj
: review+
Details | Diff | Splinter Review
62.30 KB, patch
djvj
: review+
Details | Diff | Splinter Review
29.61 KB, patch
djvj
: review+
Details | Diff | Splinter Review
22.09 KB, patch
djvj
: review+
Details | Diff | Splinter Review
99.89 KB, patch
djvj
: review+
Details | Diff | Splinter Review
256.56 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
Created attachment 8446906 [details] [diff] [review]
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 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.
(Reporter)

Comment 3

4 years ago
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!
(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 ;)
(Reporter)

Comment 5

4 years ago
(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.
(Reporter)

Updated

4 years ago
Assignee: nobody → vstamour
(Reporter)

Updated

4 years ago
Attachment #8446906 - Attachment is obsolete: true
(Reporter)

Comment 6

4 years ago
Created attachment 8447472 [details] [diff] [review]
0001-Add-a-toString-function-to-typesets.patch
(Reporter)

Comment 7

4 years ago
Created attachment 8447473 [details] [diff] [review]
0002-Instrument-Ion-to-record-optimization-information.patch
(Reporter)

Comment 8

4 years ago
Created attachment 8447474 [details] [diff] [review]
0003-Merge-inlined-script-s-optimization-info-with-their-.patch
(Reporter)

Comment 9

4 years ago
Created attachment 8447476 [details] [diff] [review]
0004-Copy-optimization-info-over-to-SPSProfiler-objects.patch
(Reporter)

Comment 10

4 years ago
Created attachment 8447477 [details] [diff] [review]
0005-Add-a-mulBy5-macro-instruction.patch
(Reporter)

Comment 11

4 years ago
Created attachment 8447478 [details] [diff] [review]
0006-Fix-typo-in-comment-in-macro-assembler.patch
(Reporter)

Comment 12

4 years ago
Created attachment 8447479 [details] [diff] [review]
0007-Push-pointer-to-optimization-information-on-the-SPS-.patch
(Reporter)

Comment 13

4 years ago
Created attachment 8447481 [details] [diff] [review]
0008-Sample-optimization-information-when-profiling.patch
(Reporter)

Comment 14

4 years ago
Split into smaller patches.
(Reporter)

Updated

4 years ago
Attachment #8447476 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8447477 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8447478 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8447479 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #8447481 - Attachment is obsolete: true
(Reporter)

Comment 15

4 years ago
Created attachment 8448379 [details] [diff] [review]
0004-Add-a-mulBy5-macro-instruction.patch
(Reporter)

Comment 16

4 years ago
Created attachment 8448380 [details] [diff] [review]
0005-Fix-typo-in-comment-in-macro-assembler.patch
(Reporter)

Comment 17

4 years ago
Created attachment 8448381 [details] [diff] [review]
0006-Commit-explaining-allocation-behavior-of-optimizatio.patch
(Reporter)

Comment 18

4 years ago
Created attachment 8448382 [details] [diff] [review]
0007-Have-counter-that-identifies-compiles.patch
(Reporter)

Comment 19

4 years ago
Created attachment 8448384 [details] [diff] [review]
0008-Store-compilation-IDs-in-SPS-profiler-objects.patch
(Reporter)

Comment 20

4 years ago
Created attachment 8448385 [details] [diff] [review]
0009-Push-compile-IDs-to-the-SPS-stack.patch
(Reporter)

Comment 21

4 years ago
Created attachment 8448386 [details] [diff] [review]
0010-Sample-compile-IDs-when-profiling.patch
(Reporter)

Comment 22

4 years ago
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.
(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.
(Reporter)

Comment 24

4 years ago
Created attachment 8448940 [details] [diff] [review]
0002-Instrument-Ion-to-record-optimization-information.patch
Attachment #8447473 - Attachment is obsolete: true
(Reporter)

Comment 25

4 years ago
Created attachment 8448942 [details] [diff] [review]
0007-Have-counter-that-identifies-compiles.patch
Attachment #8448382 - Attachment is obsolete: true
(Reporter)

Comment 26

4 years ago
Created attachment 8448943 [details] [diff] [review]
0008-Emit-profiler-events-containing-optimization-informa.patch
Attachment #8448384 - Attachment is obsolete: true
(Reporter)

Comment 27

4 years ago
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.
(Reporter)

Comment 28

4 years ago
Created attachment 8448947 [details] [diff] [review]
0009-Push-compile-IDs-to-the-SPS-stack.patch
Attachment #8448385 - Attachment is obsolete: true
(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
(Reporter)

Comment 30

4 years ago
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!
(Reporter)

Comment 31

4 years ago
Created attachment 8454796 [details] [diff] [review]
0002-Instrument-Ion-to-record-optimization-information.patch
Attachment #8448940 - Attachment is obsolete: true
(Reporter)

Comment 32

4 years ago
Created attachment 8454797 [details] [diff] [review]
0006-Comment-explaining-allocation-behavior-of-optimizati.patch
Attachment #8448381 - Attachment is obsolete: true
(Reporter)

Comment 33

4 years ago
Created attachment 8454798 [details] [diff] [review]
0011-Make-object-type-printing-available-outside-of-debug.patch
(Reporter)

Comment 34

4 years ago
Created attachment 8454800 [details] [diff] [review]
0012-Allow-printing-constructor-when-printing-object-type.patch
(Reporter)

Comment 35

4 years ago
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/
(Reporter)

Comment 36

4 years ago
Created attachment 8463515 [details] [diff] [review]
0002-Instrument-Ion-to-record-optimization-information.patch
Attachment #8454796 - Attachment is obsolete: true
(Reporter)

Comment 37

4 years ago
Created attachment 8463516 [details] [diff] [review]
0011-Make-object-type-printing-available-outside-of-debug.patch
Attachment #8454798 - Attachment is obsolete: true
(Reporter)

Comment 38

4 years ago
Created attachment 8463517 [details] [diff] [review]
0012-Allow-printing-constructor-when-printing-object-type.patch
Attachment #8454800 - Attachment is obsolete: true
(Reporter)

Comment 39

4 years ago
Minor fixes.
(Reporter)

Comment 40

4 years ago
Created attachment 8465863 [details] [diff] [review]
0013-Allow-logging-optimization-attempts-with-no-associat.patch
(Reporter)

Comment 41

4 years ago
Created attachment 8465865 [details] [diff] [review]
0014-Add-helper-to-log-MIR-types.patch
(Reporter)

Comment 42

4 years ago
Created attachment 8465866 [details] [diff] [review]
0015-Add-optimization-logging-for-getelem.patch
(Reporter)

Comment 43

4 years ago
Created attachment 8465867 [details] [diff] [review]
0016-Add-optimization-logging-for-setelem.patch
(Reporter)

Comment 44

4 years ago
Created attachment 8465868 [details] [diff] [review]
0017-Have-JSScript-ids-be-meaningful-outside-of-debug-mod.patch
(Reporter)

Comment 45

4 years ago
Added instrumentation for getelem and setelem.
(Reporter)

Comment 46

4 years ago
Created attachment 8466331 [details] [diff] [review]
0015-Add-optimization-logging-for-getelem.patch
(Reporter)

Updated

4 years ago
Attachment #8465866 - Attachment is obsolete: true
(Reporter)

Comment 47

4 years ago
Created attachment 8466334 [details] [diff] [review]
0016-Add-optimization-logging-for-setelem.patch
Attachment #8465867 - Attachment is obsolete: true
(Assignee)

Comment 48

4 years ago
Taking over hacking on this now that Vincent's internship is over.
Assignee: stamourv → shu

Updated

4 years ago
Blocks: 1068885
(Assignee)

Comment 49

4 years ago
Created attachment 8514649 [details] [diff] [review]
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.
Attachment #8514649 - Flags: review?(kvijayan)
(Assignee)

Comment 50

4 years ago
Created attachment 8514650 [details] [diff] [review]
Part 2: Track IonBuilder::jsop_getprop optimization.
(Assignee)

Comment 51

4 years ago
Created attachment 8514653 [details]
compute_encs.py

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

Updated

4 years ago
Depends on: 1082875
(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.
(Assignee)

Comment 53

4 years ago
(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.
(Assignee)

Comment 54

4 years ago
Created attachment 8540457 [details] [diff] [review]
Infrastructure: Optimization strategy tracking infrastructure.

Un-bitrotted.
Attachment #8514649 - Attachment is obsolete: true
Attachment #8514649 - Flags: review?(kvijayan)
Attachment #8540457 - Flags: review?(kvijayan)
(Assignee)

Comment 55

4 years ago
Created attachment 8540458 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_getprop optimization.
Attachment #8514650 - Attachment is obsolete: true
(Assignee)

Comment 56

4 years ago
Created attachment 8540459 [details] [diff] [review]
Infrastructure: Track type info used by optimizations.
(Assignee)

Comment 57

4 years ago
Created attachment 8540460 [details] [diff] [review]
Infrastructure: Track Ion aborts.
(Assignee)

Comment 58

4 years ago
Created attachment 8540461 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setprop optimization.
(Assignee)

Comment 59

4 years ago
Created attachment 8542770 [details] [diff] [review]
Instrumentation: Track calls.
(Assignee)

Comment 60

4 years ago
Created attachment 8542771 [details] [diff] [review]
Infrastructure: Optimization strategy tracking infrastructure.
Attachment #8540457 - Attachment is obsolete: true
Attachment #8540459 - Attachment is obsolete: true
Attachment #8540457 - Flags: review?(kvijayan)
(Assignee)

Comment 61

4 years ago
Created attachment 8548055 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_getprop optimization.
Attachment #8540458 - Attachment is obsolete: true
(Assignee)

Comment 62

4 years ago
Created attachment 8548056 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setprop optimization.
Attachment #8540461 - Attachment is obsolete: true
(Assignee)

Comment 63

4 years ago
Created attachment 8548057 [details] [diff] [review]
Instrumentation: Track calls.
Attachment #8542770 - Attachment is obsolete: true
(Assignee)

Comment 64

4 years ago
Created attachment 8548058 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_getelem optimizations.
(Assignee)

Comment 65

4 years ago
Created attachment 8550007 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setelem optimizations.
(Assignee)

Updated

4 years ago
Attachment #8542771 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8540460 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8548055 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8548056 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8548057 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8548058 - Flags: review?(kvijayan)
(Assignee)

Updated

4 years ago
Attachment #8550007 - Flags: review?(kvijayan)
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.
Attachment #8540460 - Flags: review?(kvijayan)
(Assignee)

Comment 67

4 years ago
(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.
(Assignee)

Comment 68

4 years ago
Created attachment 8552150 [details] [diff] [review]
Infrastructure: Optimization strategy tracking infrastructure.

Rebased.
Attachment #8542771 - Attachment is obsolete: true
Attachment #8542771 - Flags: review?(kvijayan)
Attachment #8552150 - Flags: review?(kvijayan)
(Assignee)

Comment 69

4 years ago
Created attachment 8552151 [details] [diff] [review]
Infrastructure: Track Ion aborts.

Rebased and fixed 'args'.
Attachment #8540460 - Attachment is obsolete: true
Attachment #8552151 - Flags: review?(kvijayan)
(Assignee)

Comment 70

4 years ago
Created attachment 8552153 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_getprop optimization.

Rebased.
Attachment #8548055 - Attachment is obsolete: true
Attachment #8548055 - Flags: review?(kvijayan)
Attachment #8552153 - Flags: review?(kvijayan)
(Assignee)

Comment 71

4 years ago
Created attachment 8552154 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setprop optimization.

Rebased.
Attachment #8548056 - Attachment is obsolete: true
Attachment #8548056 - Flags: review?(kvijayan)
Attachment #8552154 - Flags: review?(kvijayan)
(Assignee)

Comment 72

4 years ago
Created attachment 8552155 [details] [diff] [review]
Instrumentation: Track calls.

Rebased.
Attachment #8548057 - Attachment is obsolete: true
Attachment #8548057 - Flags: review?(kvijayan)
Attachment #8552155 - Flags: review?(kvijayan)
(Assignee)

Comment 73

4 years ago
Created attachment 8552156 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_getelem optimizations.

Rebased.
Attachment #8548058 - Attachment is obsolete: true
Attachment #8548058 - Flags: review?(kvijayan)
Attachment #8552156 - Flags: review?(kvijayan)
(Assignee)

Comment 74

4 years ago
Created attachment 8552157 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Rebased.
Attachment #8550007 - Attachment is obsolete: true
Attachment #8550007 - Flags: review?(kvijayan)
Attachment #8552157 - Flags: review?(kvijayan)
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..
Attachment #8552150 - Flags: review?(kvijayan)
(Assignee)

Comment 76

4 years ago
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.
(Assignee)

Updated

4 years ago
Attachment #8552150 - Flags: review?(kvijayan)
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.
Attachment #8552150 - Flags: review?(kvijayan)

Updated

4 years ago
Attachment #8552151 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 78

4 years ago
Created attachment 8554746 [details] [diff] [review]
Instrumentation: Track IonBuilder::jsop_setelem optimizations.

Addressed comments. No real changes though, per comment 76.
Attachment #8552150 - Attachment is obsolete: true
Attachment #8554746 - Flags: review?(kvijayan)
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.
Attachment #8552153 - Flags: review?(kvijayan) → review+

Updated

4 years ago
Attachment #8552154 - Flags: review?(kvijayan) → review+
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?
Attachment #8552155 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 81

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

Oops, accidentally uploaded 2 copies of this patch.
Attachment #8554746 - Attachment is obsolete: true
Attachment #8554746 - Flags: review?(kvijayan)
(Assignee)

Comment 82

4 years ago
Created attachment 8554895 [details] [diff] [review]
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.
Attachment #8554895 - Flags: review?(kvijayan)
(Assignee)

Comment 83

4 years ago
(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.
(Assignee)

Comment 84

4 years ago
Created attachment 8554903 [details] [diff] [review]
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.
Attachment #8554903 - Flags: feedback?(gary)
Attachment #8554903 - Flags: feedback?(choller)
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.
Flags: needinfo?(shu)
Attachment #8554903 - Flags: feedback?(gary)
Attachment #8554903 - Flags: feedback?(choller)
(Assignee)

Comment 86

4 years ago
Created attachment 8555107 [details] [diff] [review]
Rollup for fuzzing.

Rebased.
Attachment #8554903 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Flags: needinfo?(shu)
Attachment #8555107 - Flags: feedback?(gary)
Attachment #8555107 - Flags: feedback?(choller)
(Assignee)

Comment 87

4 years ago
Gary, to exercise the code in the patch SPS needs to be on. Can you prepend "enableSPSProfiling();" before all generated code?
(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.
Depends on: 1126210

Updated

4 years ago
Attachment #8552156 - Flags: review?(kvijayan) → review+

Updated

4 years ago
Attachment #8552157 - Flags: review?(kvijayan) → review+
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.
Attachment #8554895 - Flags: review?(kvijayan) → review+
Comment on attachment 8555107 [details] [diff] [review]
Rollup for fuzzing.

This did not immediately blow up jsfunfuzz after an overnight run.
Attachment #8555107 - Flags: feedback?(gary) → feedback+
Depends on: 1126766
(Assignee)

Updated

4 years ago
Attachment #8555107 - Flags: feedback?(choller)
(Assignee)

Updated

4 years ago
Blocks: 1127156
Depends on: 1231925
You need to log in before you can comment on or make changes to this bug.