664769 - TI: Array.foreach (both native and self-hosted) slower with TI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

If I run https://bug602132.bugzilla.mozilla.org/attachment.cgi?id=481131 on current jaegermonkey tip, I get these numbers:

-m -n: 750 780 573 633

-m: 488 489 339 375

-j: 970 959 159 192

-m -j -p: 483 487 161 369

Note that the built-in forEach is slower with TI than with pure -m (though faster than -j, for some reason), but the self-hosted one is just really slow with TI....

Comment 1

[Jan de Mooij [:jandem]]

forEach calls this function:
--
function f(val, idx, arr) {
    arr[idx] = val * val;
}
--
The problem is that the argument types of this function are unknown, and the setelem is monitored. Monitored SETELEM's currently take a really slow path, see bug 621937 comment 9.

This also slows down the self-hosted forEach. If I add a new function (copy of f) and pass it to the self-hosted forEach, the SETELEM of the new function is not monitored and -m -n is faster than -m:

-m -n: 1558 1614 440 498
-m: 1006 1074 532 535
-j: 1840 1828 216 287

The reason -j is faster than -m -n for the self-hosted forEach is the "if (i in t)" check. After removing it -m -n is faster than -j:

-m -n: 1569 1620 122 203
-m: 972 1068 220 255
-j: 1817 1784 189 259

Filed bug 664824 for the JSOP_IN problem.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to comment #1)
> forEach calls this function:
> --
> function f(val, idx, arr) {
>     arr[idx] = val * val;
> }
> --
> The problem is that the argument types of this function are unknown, and the
> setelem is monitored. Monitored SETELEM's currently take a really slow path,
> see bug 621937 comment 9.
> 
> This also slows down the self-hosted forEach. If I add a new function (copy
> of f) and pass it to the self-hosted forEach, the SETELEM of the new
> function is not monitored and -m -n is faster than -m:
> 
> -m -n: 1558 1614 440 498
> -m: 1006 1074 532 535
> -j: 1840 1828 216 287

The native forEach marks the types of its lambda's arguments as unknown to avoid updating the types each iteration, which is bad and should get fixed (just monitor each call made from within the native).

This monitoring introduces overhead though, which we could eliminate by using a self-hosted forEach which is instantiated separately at each forEach callsite.  Polymorphism slows JM+TI down, but if it used different types at each (monomorphic, presumably) callsite then it would know precise types and could even inline the called function at really hot sites.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> If I add a new function (copy of f)

I'm not sure I follow how this helps the self-hosted version....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Or is the point that just passing a function to Array.forEach makes all later invocations of that function from other contexts slow?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Monitored SETELEM's currently take a really slow path, see bug 621937 comment 9.

Worth a new bug?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to comment #5)
> > Monitored SETELEM's currently take a really slow path, see bug 621937 comment 9.
> 
> Worth a new bug?

With the extra precision we get from type barriers, the only places I've seen this bite are when we're doing something stupid and losing precision elsewhere in the analysis (like marking the argument types as unknown).  It *could* in principle bite even if we're as precise as we can be (hot and very polymorphic code).

Comment 7

[Till Schneidereit [:till]]

Fixed by landing a self-hosted Array.forEach in bug 784294

Comment 8

[Boris Zbarsky [:bzbarsky]]

So the times I see in Firefox 16 are (in order: forEach, forEach with non-null this, manually self-hosted forEach, manually self-hosted forEach with non-null this): 

Firefox 16: 997 1025 93 162 
Fx16 no TI: 737 737 269 297 

So for the self-hosted case this was already fixed as filed.

In today's nightly I see (with Ion):

Nightly: 385 381 25 119 

but that doesn't have the fix for bug 784294 yet.  I just tried doing a build with that bug, and I get: 25 24 25 121 

So yeah, fixed no matter how you look at it.  ;)