Closed Bug 1170107 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
This patch moves the num_toString optimization to GetProperty, and makes the interpreter and Baseline call GetProperty/CallProperty instead of duplicating the logic there.
Attachment #8613464 - Flags: review?(bhackett1024)
Use GetPropertyPure when we have a primitive in GetProperty.
Attachment #8613465 - Flags: review?(bhackett1024)
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 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.
Attachment #8613464 - Flags: review?(bhackett1024) → review+
Attachment #8613465 - Flags: review?(bhackett1024) → review+
(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?
(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).
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
Flags: needinfo?(jdemooij)
https://hg.mozilla.org/mozilla-central/rev/7f328852002d
https://hg.mozilla.org/mozilla-central/rev/54936d49608b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(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.
Flags: needinfo?(jdemooij) → needinfo?(hv1989)
Bisecting gave another bug. This bug isn't the cause of http://arewefastyet.com/regressions/#/regression/1711631
Flags: needinfo?(hv1989)
You need to log in before you can comment on or make changes to this bug.