Closed Bug 897926 Opened 7 years ago Closed 7 years ago

IonMonkey: Instant bail when entering using OSR

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: h4writer, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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).
Blocks: 897962
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);
}
Nope, that is another issue. Filed bug 899051 + patch for this.
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)
I'll test it this week or on the next weekend.
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.
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.
I got fooled by the script size limitation.
Now I can see the bailouts again, they are still there.
Yves, can you check if this solves the issue in 0.a.d?
Assignee: general → hv1989
Attachment #799707 - Flags: feedback?(yves.gwerder)
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.
Attachment #799707 - Attachment is obsolete: true
Attachment #799707 - Flags: feedback?(yves.gwerder)
Attachment #799751 - Flags: review?(jdemooij)
Attachment #799751 - Flags: feedback?(yves.gwerder)
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."
Attachment #799751 - Flags: review?(jdemooij) → review+
I'm going to test it with 0 A.D. later today.
https://hg.mozilla.org/mozilla-central/rev/e725385400a6
https://hg.mozilla.org/mozilla-central/rev/5ed972e07d6a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
Blocks: 914342
Attachment #799751 - Flags: feedback?(yves.gwerder)
You need to log in before you can comment on or make changes to this bug.