1248412 - +169% regression on misc-basic-array-forof (Jan 12).

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

https://arewefastyet.com/#machine=28&view=single&suite=misc&subtest=basic-array-forof&start=1452598325&end=1452647879

This issue seems to be related to Bug 1187232.

The problem I saw so far seems to be that we attempt to OSR on a loop entry, but failed because the type-barrier from the OSR block fails.

This issue blocks Bug 1242462, as we attempt to OSR after a bailout, and thus do multiple bailouts from the OSR block, which emphasizes the regression.

Comment 1

[Nicolas B. Pierron [:nbp]]

Ok, the issue here is the same as in Bug 897926.  The only difference being that for-of makes a call to @@iterator, to create the iterator which we will use for calling the next function, but this iterator is not seen as being a  local, but a stack argument.

Thus, we should consider adding the stack slot type as well as the arguments & locals.

Comment 2

[Nicolas B. Pierron [:nbp]]

Removing bailout on OSR saves 2ms over 133ms on misc-basic-array-forof (with --ion-offthread-compile=off).  So this does not seems to be the major issue here.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Prevent immediate bailout in innermost for-of loops.

Comment 4

[Nicolas B. Pierron [:nbp]]

I think that the problem is that we do not inline IsPossiblyWrappedTypedArray.  Thus we still have the branch which get the length from 2 different paths.

I can think of multiple way to solve this issue, and I think we should do all of them:


1. Check that the type set only has singletons in inlineIsTypedArray, such that we can avoid the condition:

          if (wrappingBehavior == AllowWrappedTypedArrays)
              return InliningStatus_NotInlined;

2. We change Alias Analysis such that we can LICM the  loadFixedSlot  produced by UnsafeGetFromReservedSlot(this, 0), and then do the same with the TypeBarrier and the IsPossiblyWrapperTypedArray call.  Thus, if we have no singletons, we will do 1 call instead of "n".

3. We should change  ArrayIteratorNext, such that the Jit is capable of making most of the decisions by first checking IsArray / IsTypedArray, then checking IsPossiblyWrapperTypedArray.  Doing so will surely make the self-hosted code more verbose, but would make the optimizer life easier as we no longer have branches with unknown outcome.

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> I think that the problem is that we do not inline
> IsPossiblyWrappedTypedArray.  Thus we still have the branch which get the
> length from 2 different paths.

Sorry, the problem is not much about the branches, but more about the call which is made to call the C++ function.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: inlineIsTypedArrayHelper: Check for TypedArray and Proxy classes when we allow wrapped TypedArray.

This patch add the IsTypedArrayOrProxyClass which mimic the
IsTypedArrayClass function.  The advantage of it, is that we can ALL_FALSE
is no longer ambiguous when we allow wrapped Typed Arrays as part of
IsPossiblyWrappedTypedArray intrisic.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8719837 [details] [diff] [review]
inlineIsTypedArrayHelper: Check for TypedArray and Proxy classes when we allow wrapped TypedArray.

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

::: js/src/jit/MCallOptimize.cpp
@@ +2192,5 @@
>      bool result = false;
> +    TemporaryTypeSet::ForAllResult forAllResult =
> +        wrappingBehavior == AllowWrappedTypedArrays
> +        ? types->forAllClasses(constraints(), IsTypedArrayOrProxyClass)
> +        : types->forAllClasses(constraints(), IsTypedArrayClass);

This looks wrong.  If we have AllowWrappedTypedArrays, then the first forAllClasses tests every possibility for being a typed array or proxy.  Then below, if every type IsTypedArrayOrProxyClass, we inline a constant true.  But if any type is proxy but *not* typed array, isn't that flatly wrong?

Or maybe I'm missing something.  Please convince me I'm not, and that it really is correct to treat an all-proxies result as meaning every proxy is a typed array.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: inlineIsTypedArrayHelper: Check for TypedArray and Proxy classes when we allow wrapped TypedArray.

Delta:
 - Add a test case which *should* highlight issues with the previous code.
 - Replace IsTypedArrayOrProxyClass by IsProxyClass.
 - Restore the previous checks, except that we now check for
   forAllClasses(IsProxyClass), such that we can optimize the case where:

     None of the inputs are TypedArrays, and
     None of the inputs are Proxies.

Note:
  I made the test case as fair as possible, while avoiding Type Inferrence
  aliasing issues.  On my way to check that I covered all code paths I
  noticed that when a cross comparment wrapper Proxy is monitored, we mark
  the TypeSet as being an unknownObject [1], which results in a MIXED state
  when forAllClasses is used [2].

  Thus, I was unable to test all the code paths, and when I tried to
  explicitly create Proxies, I failed to make one which makes
  IsPossiblyWrappedTypedArray return true.

  Still, as I do not understand all the things behind TI, I think the
  current code is more logical and future proof.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TypeInference.cpp#597
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/TypeInference.cpp#2288

Comment 9

[Hannes Verschore [:h4writer]]

Comment on attachment 8719775 [details] [diff] [review]
Prevent immediate bailout in innermost for-of loops.

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

R+ with the requested change.

::: js/src/jit/BaselineFrame.h
@@ -180,5 @@
>          return argv()[i];
>      }
>  
>      Value& unaliasedLocal(uint32_t i) const {
> -        MOZ_ASSERT(i < script()->nfixed());

I don't think we should remove this.
Shouldn't callers that need this, just call valueSlot() instead?

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8720344 [details] [diff] [review]
inlineIsTypedArrayHelper: Check for TypedArray and Proxy classes when we allow wrapped TypedArray.

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

The code changes look great.

But your test is a huge mass of code with very unclear steps/logic.  I understand *somehow* you're doing a bunch of testing that IsPossiblyWrappedTypedArray works correctly in all cases, but I can't for the life of me quickly identify how.  And it's very not-clear at all why it's necessary to modulo-select values and compare them, or much of anything here.

Please rewrite this test to use straightforward logic that does testing at very obvious junctures.  Assuming this test effectively tests what you want it to -- which is totally non-obvious to me -- I would dread having to divine exactly what it did, to fix it, if I happened to write a patch that broke it.  I'm sorry, but I can't allow anyone else to be subjected to that burden if I can help it.

::: js/src/jit-test/tests/self-hosting/is-possibly-wrapped-typed-array.js
@@ +19,5 @@
> +];
> +
> +// Create a new global to wrap with cross compartment wrappers.
> +var g = newGlobal();
> +g.eval(`

If this stays anywhere close to what it is now after adjustments: assign this string to a variable, then pass it to evaluate() and to g.evaluate().  (Note: not g.eval, don't use eval with its weird characteristics when evaluate is much more vanilla and you don't need eval's oddities.(  Then you don't have this duplication of code (and possibility of it not being identical, despite it appearing to be identical.

::: js/src/jit/MCallOptimize.cpp
@@ +2196,5 @@
>          // Wrapped typed arrays won't appear to be typed arrays per a
>          // |forAllClasses| query.  If wrapped typed arrays are to be considered
>          // typed arrays, a negative answer is not conclusive.  Don't inline in
>          // that case.
> +        if (wrappingBehavior == AllowWrappedTypedArrays) {

Great, this looks correct to me.

That said, this whole method is getting complicated and laborious enough that I think we're approaching the point where we shouldn't try to share code between the wrapped/non-wrapped cases.  Once this has landed, I think we should consider a followup to split this into two methods, for a lot more clarity.

Comment 11

[Nicolas B. Pierron [:nbp]]

Attachment: inlineIsTypedArrayHelper: Check for TypedArray and Proxy classes when we allow wrapped TypedArray.

Delta:
 - Remove the complex test case logic which is are to follow, at the cost of
 spinning up to 7s for running this test case with an x64 debug build.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 8721371 [details] [diff] [review]
inlineIsTypedArrayHelper: Check for TypedArray and Proxy classes when we allow wrapped TypedArray.

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

I'm certainly a fan of massively overdone testing like this, usually.  Here, tho, I think it's less than clear that we'll have perturbed TI info and codegen in just the right way to test all the different code paths.  A part of me is inclined to require you write custom-created code paths that clearly and obviously test all the cases.  But, if the overkill really *does* test all the cases, I guess it's good enough.

BUT, I'd like you to show that you really have hit all the edges.  So here's a trick to do to prove that.  Add half a dozen static Atomic<uint32_t> (or 64 on 64, if you are) to MCallOptimize.cpp.  Then in each code path arm of the inlining method, increment *one* of these Atomic<uint32_t>.  (It's good enough to test you're hitting the code paths in MCallOptimize, no need to do the same increment in JIT code.)  Finally, a shell method that'll print the values of those Atomic<uint32_t> for viewing.  If adding a call to that shell builtin at the *start* of this test prints a sequence of numbers, and a call at the end of the test prints an entirely different sequence of numbers, then I'm happy with your test.  If it's *not* the case that wholly different numbers are printed, tho, I want your test improved until it does.

::: js/src/jit-test/tests/self-hosting/is-possibly-wrapped-typed-array.js
@@ +1,1 @@
> +var isPossiblyWrappedTypedArray = getSelfHostedValue("IsPossiblyWrappedTypedArray");

Capitalize the variable name -- seems better to have consistent naming, than to be camelCaps or whatever.

@@ +24,5 @@
> +var g = newGlobal();
> +evaluate(declareSamples)
> +g.evaluate(declareSamples);
> +
> +var assert = function (value, expected) {

var assertCode = `(function (value, expected) {
    assertEq(isPossiblyWrappedTypedArray(value), expected);
    return inIon();
})`;

@@ +34,5 @@
> +    // Discard previous compilation results. This lines creates a new function
> +    // by using the code of the original one, which effectively for us, create a
> +    // new JSScript, and then a new Baseline code, and a new IonCode with new
> +    // type information as we execute it.
> +    assert = eval(uneval(assert));

And then here, |var assert = Function(assertCode);|, which nicely restricts |assert| to existing *only* inside a single closure for greater readability.  (And avoid eval and the non-standard uneval in favor of standardized things.)

The comment can be simplified a little, too:

    // Create the assert function anew every run so as not to share JIT code,
    // type information, etc.

@@ +39,5 @@
> +
> +    // Prevent Ion compilation of this function. The reason being that we do not
> +    // want to freeze the type information related to the sample vector.  If we
> +    // were to do so, we will always see a Mixed state which would prevent
> +    // optimizations of the IsPossiblyWrappedTypedArray in IonMonkey.

Tweaked:

    // Prevent Ion compilation of this function so that we don't freeze the
    // sample array's type.  If we did, IonBuilder's typed-array-length inlining
    // would always see a Mixed state, preventing IsPossiblyWrappedTypedArray
    // from being inlined.

@@ +61,5 @@
> +    var samples = [
> +        a == -1 ? undefined : allTypedArraySamples[a],
> +        b == -1 ? undefined : allOtherSamples[b],
> +        c == -1 ? undefined : g.allTypedArraySamples[c],
> +        d == -1 ? undefined : g.allOtherSamples[d]

Could you use null for these?  null/object feels nicer as a union-type than undefined/object.

@@ +76,5 @@
> +for (var a = -1; a < allTypedArraySamples.length; a++) {
> +    for (var b = -1; b < allOtherSamples.length; b++) {
> +        for (var c = -1; c < g.allTypedArraySamples.length; c++) {
> +            for (var d = -1; d < g.allOtherSamples.length; d++) {
> +                // Skip combinaisons mixing more than 2 elements from different

combinations

@@ +77,5 @@
> +    for (var b = -1; b < allOtherSamples.length; b++) {
> +        for (var c = -1; c < g.allTypedArraySamples.length; c++) {
> +            for (var d = -1; d < g.allOtherSamples.length; d++) {
> +                // Skip combinaisons mixing more than 2 elements from different
> +                // types, these are unlikely to be interesting.

s/,/, as/ or s/,/:/

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #12)
> […]  But, if the overkill really
> *does* test all the cases, I guess it's good enough.

Already verified before attaching the patch to the bug.

As mentioned in comment 8, it does verified all the code paths which are currently reachable from TI, adding additional combinations to the overkill loops does not improve the coverage.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/460195af9102
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da938156283

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/460195af9102
https://hg.mozilla.org/mozilla-central/rev/1da938156283