897926 - IonMonkey: Instant bail when entering using OSR

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

The following testcase will bail every time, until we capture some types for "val"

var test = function(val) {
  for (var i=0; i<4000; i++) {
    i -= 0.1
  }

  var test2 = function() {
    print(val);
  }
  test2();
}

test({})

The reason is that the arguments of the function doesn't have a type, since we haven't recorded them yet. (Baseline + saving types only starts at usecount > 10). So when compiling when the script gets hot at the loop, we don't have that information.

When building this script, we have a baselineFrame where we can get the actualValue from and we use that to still get types when encountering this issue.

Now the function is heavy-weighted and the "val" value is located in the callobject. So we can't get the value the normal way.

The easiest way would be to remove the guardtypes, since normally for every access to the callobject we already have typeBarriers (with right type information).

Comment 1

[Yves Gwerder]

I investigated a little further and figured out that the same problem that causes the type-barrier issue also seems to cause the bounds check issue. The problem only occurs if the loop is there inside fun.
If I pass i as first argument to fun instead of rnd, it only bails once, so the non-consecutive access is also important. I'm not sure if this needs a separate bug, so I just add it here because it seems to be very similar.

var obj = {}

fun = function(index, value)
{
for(var i=0; i<4000; i++)
  i -= 0.1;

obj[index] = value;
}
for (var i=0; i<20; i++)
{
var rnd = Math.floor(Math.random() * 3000);
fun(rnd, true);
}

Comment 2

[Hannes Verschore [:h4writer]]

Nope, that is another issue. Filed bug 899051 + patch for this.

Comment 3

[Hannes Verschore [:h4writer]]

Since bug 902508 landed, I cannot reproduce the JS testcase. So it could be solved already.
Yves could you test if the issue is also fixed in 0.a.d. on a recent build? (since yesterday)

Comment 4

[Yves Gwerder]

I'll test it this week or on the next weekend.

Comment 5

[Yves Gwerder]

Unfortunately I couldn't test it yet.
I had to work around a linking issue when updating and now there are a lot of API change to catch up. I should have time next thursday or on the next weekend.

Comment 6

[Yves Gwerder]

The peaks in the performance graph are the same but the typebarrier bailouts in qbot-wc/map-module.js:39 are gone. I'll see if I can figure out more.

Comment 7

[Yves Gwerder]

I got fooled by the script size limitation.
Now I can see the bailouts again, they are still there.

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: bug897926-remove-aliased-typebarriers

Yves, can you check if this solves the issue in 0.a.d?

Comment 9

[Hannes Verschore [:h4writer]]

Attachment: bug897926-remove-aliased-typebarriers

Functionally same patch, but now ready for review.

The changes in "newPendingLoopHeader" are pure cosmetic. I'm fine to leave it as is, but I think it is better now.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 799751 [details] [diff] [review]
bug897926-remove-aliased-typebarriers

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

Nice catch!

::: js/src/jit/CompileInfo.h
@@ +230,5 @@
> +                return true;
> +            return false;
> +        }
> +
> +        return false;

Nit: JS_ASSERT(index >= firstStackSlot());

::: js/src/jit/IonBuilder.cpp
@@ +5847,5 @@
> +            // the value from the baseline frame.
> +            if (info().isSlotAliased(i))
> +                continue;
> +
> +            // Values above the locals are also not in the baseline frame.

Nit: they are in the baseline frame. Maybe "Don't bother with expression stack values. The stack should be empty except for let variables (not Ion-compiled) or iterators."

Comment 11

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e725385400a6

Comment 12

[Yves Gwerder]

I'm going to test it with 0 A.D. later today.

Comment 13

[Hannes Verschore [:h4writer]]

Fix for ss-string-fasta regression.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ed972e07d6a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e725385400a6
https://hg.mozilla.org/mozilla-central/rev/5ed972e07d6a

Comment 15

[Yves Gwerder]

We have already discussed this on irc, but to sum it up:
Although the performance issues in map-module.js:39 for 0 A.D. aren't fixed, this bug seems to be fixed.
The bailout signature before the patch was:
[Bailouts]  bailing from bytecode: loopentry, MIR: typebarrier [73], LIR: typebarrier [85]

Now it doesn't bailout less but there are two different signatures:
[Bailouts]  bailing from bytecode: getaliasedvar, MIR: typebarrier [818], LIR: typebarrier [1308]
[Bailouts]  bailing from bytecode: nop, MIR: constant [0], LIR: osipoint [1272]

These bailouts continue and happen over and over again.
I'll try writing a testcase to reproduce these bailouts this weekend and then create a new ticket.