Closed
Bug 1175466
Opened 9 years ago
Closed 9 years ago
Allocate arguments objects in the nursery
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
Most benchmarks don't create (non-lazy) arguments objects, but some websites and frameworks use them a lot. We can do much better than we're doing now.
This patch lets us nursery allocate ArgumentsObject and its ArgumentsData. It's nice because (1) arguments objects are usually short-lived, so they really benefit from GGC and (2) we avoid a (slow) malloc for the ArgumentsData.
For the micro-benchmark below I get (Mac 32-bit):
before: 307 ms
after: 172 ms
The ArgumentsObject allocation path is still pretty slow but this is a good start.
var args;
function g() {
args = arguments;
}
function f() {
for (var i=0; i<1000000; i++)
g(i);
}
var t = new Date;
f();
print(new Date - t);
Attachment #8623590 -
Flags: review?(terrence)
Comment 1•9 years ago
|
||
Comment on attachment 8623590 [details] [diff] [review]
Patch
Review of attachment 8623590 [details] [diff] [review]:
-----------------------------------------------------------------
Nice!
Attachment #8623590 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Bleh, it was green this morning but some GC changes broke it in the meantime:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f942c1cd0d0
Assignee | ||
Comment 4•9 years ago
|
||
So with this patch + bug 1175642 we assert in assertHasValueEdge with the following testcase:
function f() {
arguments[0] = {};
arguments[0] = {};
}
f(1);
If the argument's HeapValue is in the nursery, ValueEdge::maybeInRememberedSet will return false and we don't add any ValueEdge to the StoreBuffer. Then later on assertHasValueEdge fails.
Changing assertHasValueEdge to this fixes it:
void assertHasValueEdge(Value* vp) const {
MOZ_ASSERT_IF(ValueEdge(vp).maybeInRememberedSet(nursery_), bufferVal.has(ValueEdge(vp)));
}
Terrence does this make sense? Is there a better way to fix this?
Flags: needinfo?(terrence)
Comment 5•9 years ago
|
||
Yes, I think the assertion just does not consider nursery->nursery edges. Please fix it inline, or I'll whip up a separate patch today.
Flags: needinfo?(terrence)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8625892 -
Flags: review?(terrence)
Comment 7•9 years ago
|
||
Comment on attachment 8625892 [details] [diff] [review]
Fix asserts
Review of attachment 8625892 [details] [diff] [review]:
-----------------------------------------------------------------
Sorry for the bustage!
Attachment #8625892 -
Flags: review?(terrence) → review+
Comment 9•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•