1170107 - Don't allocate a wrapper object when doing primitive.foo

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

The benchmark in bug 1169501 (which uses Facebook's Immutable.js library) ends up allocating a lot of objects because it does v.valueOf and v.equals on primitives. Because these sites are very polymorphic and Ion doesn't have GETPROP caches that take primitives, Ion isn't always able to optimize this.

Bug 771865 added an optimization to avoid creating a NumberObject when doing (3).toString. However, this optimization is (1) very narrow; it doesn't work for v.valueOf we have here and (2) interpreter-only. Nowadays it's not too hard to make this optimization work in a lot more cases though, by doing a pure lookup.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Refactoring

This patch moves the num_toString optimization to GetProperty, and makes the interpreter and Baseline call GetProperty/CallProperty instead of duplicating the logic there.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Optimize more cases

Use GetPropertyPure when we have a primitive in GetProperty.

Comment 3

[Jan de Mooij [:jandem]]

These patches improve the micro-benchmark below from ~2000 ms to ~1500 ms.

1500 ms is still pretty slow though; the problem is also that Ion only optimizes native calls when there's a single target, and here we have two natives (str_toString and num_toString). Will look into that as well.

(--no-ion is 6x faster, 240 ms, and interpreter-only is about as fast as Ion...)

function f() {
    var res;
    for (var i=0; i<10000000; i++) {
	var v = (i & 1) ? "foo" : 0;
	res = v.toString();
    }
    return res;
}
var t = new Date;
f();
print(new Date - t);

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 8613464 [details] [diff] [review]
Part 1 - Refactoring

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

Nice cleanup.  The GetProperty stuff in Interpreter.cpp is a mess.  It would be nice if GetPropertyOperation and GetProperty could be merged.  I thought the point of these FooBarOperation functions was so that the logic could be commoned between the interpreter and JITs, but when these functions take an InterpreterFrame* that can't be done.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f328852002d
https://hg.mozilla.org/integration/mozilla-inbound/rev/54936d49608b

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #4)
> Nice cleanup.  The GetProperty stuff in Interpreter.cpp is a mess.  It would
> be nice if GetPropertyOperation and GetProperty could be merged.  I thought
> the point of these FooBarOperation functions was so that the logic could be
> commoned between the interpreter and JITs, but when these functions take an
> InterpreterFrame* that can't be done.

Yeah, the lazy arguments stuff is annoying. Maybe we can rename GetProperty/CallProperty to GetPropertyOperation/CallPropertyOperation, and rename the interpreter-only GetPropertyOperation to GetProperty?

Comment 7

[Brian Hackett [Laid off!]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> Yeah, the lazy arguments stuff is annoying. Maybe we can rename
> GetProperty/CallProperty to GetPropertyOperation/CallPropertyOperation, and
> rename the interpreter-only GetPropertyOperation to GetProperty?

Hmm, GetProperty is such a generic name, it doesn't seem like it should be used for interpreter specific stuff.  I think we could have a single generic GetPropertyOperation function that doesn't take an InterpreterFrame*, and then put the logic for lazy arguments either directly in the JSOP_GETPROP case in the interpreter (since GetPropertyOperation is only called there) or in a helper that is called by the JSOP_GETPROP case if IsOptimizedArguments(fp, lval).

Comment 8

[Guilherme Lima]

These patches apparently regressed kraken-darkroom, but only on Mac 32-bit. Is that possible? http://arewefastyet.com/#machine=28&view=single&suite=kraken&subtest=darkroom

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f328852002d
https://hg.mozilla.org/mozilla-central/rev/54936d49608b

Comment 10

[Hannes Verschore [:h4writer]]

(In reply to Guilherme Lima from comment #8)
> These patches apparently regressed kraken-darkroom, but only on Mac 32-bit.
> Is that possible?
> http://arewefastyet.com/#machine=28&view=single&suite=kraken&subtest=darkroom
Needinfo myself to try to reproduce on the original slave. Jan noted he couldn't reproduce locally.

Comment 11

[Hannes Verschore [:h4writer]]

Bisecting gave another bug. This bug isn't the cause of http://arewefastyet.com/regressions/#/regression/1711631