Closed
Bug 1341071
Opened 8 years ago
Closed 8 years ago
Fix Ion IC post write barriers
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
37.68 KB,
patch
|
jonco
:
review+
nbp
:
review+
|
Details | Diff | Splinter Review |
MPostWriteBarrier instructions are added in IonBuilder when we add a SetProp/SetElem IC.
This works but is a bit weird because in a lot of cases (setters, proxies, the store happening in the VM, etc) it will add a barrier and fill the store buffer for no good reason. Furthermore, we will optimize more things in the future and we might do the store on an object that's different from the object we have in IonBuilder.
To fix these issues the IC stubs should have their own post barriers (like the Baseline stubs).
Assignee | ||
Comment 1•8 years ago
|
||
Instead of adding a post barrier in IonBuilder for the SetProp/SetElem IC, this adds the barrier to the IC stubs we attach, like Baseline.
This avoids adding unnecessary barriers (and slowing down nursery GCs) when all we're doing is pass a nursery object to a setter or proxy trap, or when we don't attach IC stubs but do the store in the VM.
The patch also changes the IC code to use the per-element barrier optimization (see PostWriteElementBarrier) when possible. Before, we only did that in Ion and only for int32 indexes. The micro-benchmark below improves from 3200 ms to 739 ms because of this.
We should add fewer objects to the whole cell buffer than before, so this should have a positive effect on our nursery GC times.
function f() {
var a = [];
var t = new Date;
for (var i=0; i<10000000; i++) {
a["" + i] = {};
if (i > 0)
a[i-1] = null;
}
print(new Date - t);
}
f();
Assignee | ||
Updated•8 years ago
|
Attachment #8844041 -
Flags: review?(nicolas.b.pierron)
Comment 2•8 years ago
|
||
Comment on attachment 8844041 [details] [diff] [review]
Patch
Review of attachment 8844041 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/IonBuilder.cpp
@@ +11675,1 @@
> bool needsBarrier = objTypes->propertyNeedsBarrier(constraints(), NameToId(name));
I guess |needsBarrier| is a different barrier - looking at the code it's for the pre barrier. Can we rename this to |needsPreBarrier|? Also below.
::: js/src/jit/VMFunctions.cpp
@@ +665,5 @@
> }
>
> static const size_t MAX_WHOLE_CELL_BUFFER_SIZE = 4096;
>
> +template <bool IndexInBounds>
Could you make this an enum to make it clearer at the callsite what's happening?
@@ +672,5 @@
> {
> MOZ_ASSERT(!IsInsideNursery(obj));
> +
> + if (IndexInBounds) {
> + MOZ_ASSERT(uint32_t(index) < obj->as<NativeObject>().getDenseInitializedLength());
It's not clear to me how we know that obj is always a native object at this point, but I expect that's handled by the JIT.
Attachment #8844041 -
Flags: review?(jcoppeard) → review+
Updated•8 years ago
|
Whiteboard: [qf:investigate]
Comment 3•8 years ago
|
||
Comment on attachment 8844041 [details] [diff] [review]
Patch
Review of attachment 8844041 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, this is indeed cleaner to have the post-write-barrier next to the store at the assembly level, instead of being at the IonBuilder level.
::: js/src/jit/CodeGenerator.cpp
@@ +3855,5 @@
> masm.mov(ImmPtr(GetJitContext()->runtime), runtimereg);
> masm.passABIArg(runtimereg);
> masm.passABIArg(objreg);
> masm.passABIArg(indexreg);
> + masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, (PostWriteElementBarrier<false>)));
nit: Add a AutoAssertNoGC in PostWriteElementBarrier function.
Attachment #8844041 -
Flags: review?(nicolas.b.pierron) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/51bffdbb6a30
Fix Ion IC post barriers to be part of the IC stubs instead of adding them in IonBuilder. r=jonco,nbp
Comment 5•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•3 years ago
|
Performance Impact: --- → ?
Whiteboard: [qf:investigate]
You need to log in
before you can comment on or make changes to this bug.
Description
•