IonMonkey: Instant bail when entering using OSR

RESOLVED FIXED in mozilla26

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

(Blocks: 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 897962

Comment 1

5 years ago
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);
}
(Assignee)

Comment 2

5 years ago
Nope, that is another issue. Filed bug 899051 + patch for this.
(Assignee)

Comment 3

5 years ago
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

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

Comment 5

5 years ago
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

5 years ago
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

5 years ago
I got fooled by the script size limitation.
Now I can see the bailouts again, they are still there.
(Assignee)

Comment 8

5 years ago
Created attachment 799707 [details] [diff] [review]
bug897926-remove-aliased-typebarriers

Yves, can you check if this solves the issue in 0.a.d?
Assignee: general → hv1989
Attachment #799707 - Flags: feedback?(yves.gwerder)
(Assignee)

Comment 9

5 years ago
Created attachment 799751 [details] [diff] [review]
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.
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+

Comment 12

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Comment 15

5 years ago
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.

Updated

5 years ago
Blocks: 914342
(Assignee)

Updated

5 years ago
Attachment #799751 - Flags: feedback?(yves.gwerder)
You need to log in before you can comment on or make changes to this bug.