GenerationalGC: NewObjectCache::newObjectFromHit allocates in the tenured heap when the nursery is full

RESOLVED FIXED in mozilla30



JavaScript Engine
4 years ago
4 years ago


(Reporter: jandem, Assigned: terrence)


(Blocks: 1 bug)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(2 attachments, 1 obsolete attachment)



4 years ago
Created attachment 808590 [details]
Shell testcase

NewObjectCache::newObjectFromHit does:

    JSObject *obj = js_NewGCObject<NoGC>(cx, entry->kind, heap);

The NoGC is bad: when the nursery is full, we don't trigger minor GCs and happily allocate in the tenured heap.

I'm attaching a testcase (based on bug 918746) that runs almost twice as fast if I disable the NewObjectCache lookup in NewArray. We really want to nursery-allocate these arrays to avoid slow malloc/realloc for the elements.

Comment 1

4 years ago
Terrence, this also affects the micro-benchmark in bug 874580 comment 8. A GGC build is at least 2x slower but after disabling the NewObjectCache lookup in NewObjectWithClassProtoCommon we're suddenly 3x faster.

Requesting needinfo: I keep running into this and it probably affects non-micro-benchmarks as well.
Flags: needinfo?(terrence)


4 years ago
Blocks: 874580

Comment 2

4 years ago
Good catch, that is indeed quite bad! I did not notice that newObjectFromHit used NoGC.

I think this is only used in spots that allocate, so switching that to MayGC should not be that hard. The static rooting analysis (working on try now) would probably help make sure we don't make any mistakes.
Flags: needinfo?(terrence)

Comment 3

4 years ago
Created attachment 8368899 [details] [diff] [review]

This turned out to be fairly annoying to implement cleanly: we want to GC, but still return null, which is at total cross purposes to our current allocators. I ended up implementing a new allocator function specially for the new case; an extra template arg would have worked with less code, but would have made both paths totally incomprehensible.

The attached patch takes my local times on the testcast in this bug from 620ms -> 185ms. Octane does not appear to be affected, but we should keep an eye on *Latency when this lands just in case.
Assignee: general → terrence
Attachment #8368899 - Flags: review?(jdemooij)

Comment 5

4 years ago
Comment on attachment 8368899 [details] [diff] [review]

Review of attachment 8368899 [details] [diff] [review]:

Nice, thanks!

::: js/src/jsgcinlines.h
@@ +529,5 @@
> +    if (ShouldNurseryAllocate(cx->nursery(), kind, heap)) {
> +        size_t thingSize = Arena::thingSize(kind);
> +
> +        JS_ASSERT(thingSize == Arena::thingSize(kind));

Nit: can remove the assert in this case.
Attachment #8368899 - Flags: review?(jdemooij) → review+

Comment 6

4 years ago
(In case you missed it, the static analysis found some new hazards with this patch.)

Comment 7

4 years ago
Created attachment 8369585 [details] [diff] [review]

The one case I looked at rooted everything up front, but I still should have realized the templateObject pointer would be a "hazard" even though it doesn't even point into the heap.

This patch uses AllowGC to delay rooting in all cases until after a cache hit fails, which hopefully will shave a few more nanoseconds off the common case.
Attachment #8368899 - Attachment is obsolete: true
Attachment #8369585 - Flags: review+
I see 13% improvement on ss-aes. Bringing ggc on par with non-ggc on this benchmark
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.