Closed Bug 1175466 Opened 9 years ago Closed 9 years ago

Allocate arguments objects in the nursery

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)

Attached patch Patch (deleted) — 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 on attachment 8623590 [details] [diff] [review] Patch Review of attachment 8623590 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8623590 - Flags: review?(terrence) → review+
Bleh, it was green this morning but some GC changes broke it in the meantime: https://hg.mozilla.org/integration/mozilla-inbound/rev/5f942c1cd0d0
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)
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)
Attached patch Fix asserts (deleted) — Splinter Review
Attachment #8625892 - Flags: review?(terrence)
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+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: