Closed Bug 1113240 Opened 5 years ago Closed 5 years ago

Bad performance with nursery-allocated getters/setters

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

We're at least 25x (!) slower than V8 on the Shumway v8-deltablue benchmark. We're spending a ton of time in Ion's GetProperty IC because some scripted getters are not optimized/inlined.

The problem is this:

(1) Baseline does not attach a getter/setter call stub if the getter/setter JSFunction or the holder object is in the nursery.
(2) IonBuilder's scripted getter/setter optimizations are based on the Baseline stubs but because we didn't attach any, Ion doesn't optimize the getters.
(3) Even if a nursery GC happens, we're still stuck with the slow Ion code.

Deltablue is 6x faster with --no-ggc and --no-ion is also 2-3x faster.

The micro-benchmark below:

js --no-ggc:    4 ms
d8         :   64 ms
js         : 3060 ms

function f() {
    var o2 = Object.create({get foo() { return 3; }});
    var res = 0;
    for (var i=0; i<10000000; i++)
	res = o2.foo;
    return res;
}
var t = new Date;
f();
print(new Date - t);
Also, we could probably fix this one:

> (1) Baseline does not attach a getter/setter call stub if the getter/setter
> JSFunction or the holder object is in the nursery.

But the nursery pointers will still be a problem for Ion compilation (it's not allowed to hold nursery pointers as minor GC's can run in parallel with off-thread compilation...)
Hmm.  We'd made this case not permanently mark the pc as unoptimizable in bug 1091795, but I guess in this case the ion-compile happens before the nursery GC and we don't discard the bogus ioncode when the gc happens?
(In reply to Vacation Dec 15-31 from comment #2)
> but I guess in this case the ion-compile happens before the
> nursery GC and we don't discard the bogus ioncode when the gc happens?

Yes. But also: the nursery GC may never happen while we're running the benchmark, if we don't allocate much, like the micro-benchmark case in comment 0. I don't see a nice and clean fix unfortunately :(
How insane would it be to trigger a nursery GC before starting a parallel ion compile?
(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Vacation Dec 15-31 from comment #2)
> > but I guess in this case the ion-compile happens before the
> > nursery GC and we don't discard the bogus ioncode when the gc happens?
> 
> Yes. But also: the nursery GC may never happen while we're running the
> benchmark, if we don't allocate much, like the micro-benchmark case in
> comment 0. I don't see a nice and clean fix unfortunately :(

If this is unlikely to affect much real-world code, then I'd be mostly fine with not fixing it. Nobody compares our performance to v8's on this version of DeltaBlue :)

Then again, it seems somewhat likely to actually affect not only Shumway but also other real-world content.

(In reply to Vacation Dec 15-31 from comment #4)
> How insane would it be to trigger a nursery GC before starting a parallel
> ion compile?

I guess one problem with that would be that we might prematurely tenure objects we normally wouldn't, and shouldn't.
Could we detect nursery pointers in IonBuilder and use a placeholder immediate (invalid but easily indexed object pointers like 0x1, 0x2, ...) in the rest of Ion compilation, then substitute the actual pointer (whether still in the nursery, or tenured) on the main thread during linking?  This would require some new machinery but seems like it would be fairly clean.
(In reply to Till Schneidereit [:till] from comment #5)
> Then again, it seems somewhat likely to actually affect not only Shumway but
> also other real-world content.

Yes and getters/setters are supported by modern IE so will likely be used more and more.

(In reply to Brian Hackett (:bhackett) from comment #6)
> Could we detect nursery pointers in IonBuilder and use a placeholder
> immediate (invalid but easily indexed object pointers like 0x1, 0x2, ...) in
> the rest of Ion compilation, then substitute the actual pointer (whether
> still in the nursery, or tenured) on the main thread during linking?  This
> would require some new machinery but seems like it would be fairly clean.

Thanks, great idea! We could have a Vector for each compilation that's only touched on the main thread and updated by the minor GC. Then linking has to update the script's constant list and the JitCode but that should be doable.
What would be required to add Nursery pointers to Ion ICs?

About the vector solution:
 - Does this mean that we would have to tenure all Nursery pointers which are in this vector, or we are fine about having Nursery pointers baked into Ion?
 - If we have a vector, how would we investigate the shape / type of the object, to know that we have getters / properties? Should we register these meta-data ahead, or just lock, such that we can even read other Nursery objects properties?
 - Do we need another mark function, and a section for nursery pointers?
 - Do we need extra barriers around the manipulation of these pointers?

Would it be possible to have a shell test case in the assorted tests?
(In reply to Nicolas B. Pierron [:nbp] from comment #8)
> What would be required to add Nursery pointers to Ion ICs?

This works already but you need to use ImmMaybeNurseryPtr.

>  - Does this mean that we would have to tenure all Nursery pointers which
> are in this vector, or we are fine about having Nursery pointers baked into
> Ion?

Ideally we'd support nursery pointers in Ion scripts I think.

>  - If we have a vector, how would we investigate the shape / type of the
> object, to know that we have getters / properties? Should we register these
> meta-data ahead, or just lock, such that we can even read other Nursery
> objects properties?

IonBuilder runs on the main thread, it'd probably have to store everything codegen needs so we don't have to dereference the JSObject..

>  - Do we need another mark function, and a section for nursery pointers?
>  - Do we need extra barriers around the manipulation of these pointers?

If those pointers are stored in the Ion script/code, we'll have to add a store buffer entry for the whole script/JitCode or one for each pointer.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
This mostly implements the idea described in comment 6. It fixes the testcase in comment 0 and deltablue.swf is about 2x faster, about as fast as --no-ggc. This needs more getter/setter optimizations in IonBuilder, patches for that coming up after this is in.

So IonBuilder will have a Vector of nursery objects. This Vector is only accessed on the main thread. Minor GCs trace these Vectors and after compilation is done we patch the dummy ImmGCPtrs to their actual values (on the main thread).

If we have a nursery object in IonBuilder, we add MNurseryObject, which contains an index into this Vector. Initially I used MConstant with a magic JSObject*, but that's a lot more complicated because MConstant is used in a ton of places. For instance, with MNurseryObject we can guarantee snapshots and the IonScript's constant list never have nursery JSObject* constants.

I think this turned out pretty well. There are some TI bits I'm not sure about, like I had to add a TypeObject method that gets the proto without asserting it's tenured, do you think we should use it in other places?
Attachment #8555916 - Flags: review?(bhackett1024)
Comment on attachment 8555916 [details] [diff] [review]
Patch

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

protoMaybeInNursery looks fine for now, though with this mechanism in the future we should be able to remove hasTenuredProto() and merge TypeObjectKey::protoMaybeInNursery() and TypeObjectKey::proto().

::: js/src/jit-test/tests/ion/nursery-getter-setter.patch
@@ +1,4 @@
> +function bar(i) {
> +    if (!i)
> +	with (this) {}; // Don't inline.
> +    if (i === 1101) {

This seems fragile.

::: js/src/jsinferinlines.h
@@ +124,5 @@
> +/* static */ inline TypeObjectKey *
> +TypeObjectKey::get(JSObject *obj)
> +{
> +    MOZ_ASSERT(obj);
> +    if (!IsInsideNursery(obj))

This test should be:

if (obj->hasSingletonType())

Singleton objects are always tenured, and non-singleton objects should not be added to type sets.
Attachment #8555916 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb01144424b5

(In reply to Brian Hackett (:bhackett) from comment #11)
> protoMaybeInNursery looks fine for now, though with this mechanism in the
> future we should be able to remove hasTenuredProto() and merge
> TypeObjectKey::protoMaybeInNursery() and TypeObjectKey::proto().

Cool, next week I'll see if hasTenuredProto() is slowing us down atm.
https://hg.mozilla.org/mozilla-central/rev/cb01144424b5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attached patch Follow-up fixSplinter Review
I was working on another getter/setter patch and it exposed a small problem with this patch: when inlining, IonBuilder needs to use the outer builder's object vector.

I think at some point we should move this Vector and other stuff we don't need for each inline builder to a new class, or maybe IonBuilder should not inherit from MIRGenerator, but for now this will do.
Attachment #8557910 - Flags: review?(bhackett1024)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8557910 - Flags: review?(bhackett1024) → review+
Duplicate of this bug: 1128476
Duplicate of this bug: 1128901
https://hg.mozilla.org/mozilla-central/rev/a03855955349
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.