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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: 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

[Jan de Mooij [:jandem]]

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.

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Terrence Cole [:terrence]]

Attachment: fix_newobjectfromhit_allocation-v0.diff

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.

Comment 4

[Terrence Cole [:terrence]]

https://tbpl.mozilla.org/?tree=Try&rev=dfbc98889c5a

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8368899 [details] [diff] [review]
fix_newobjectfromhit_allocation-v0.diff

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

Nice, thanks!

::: js/src/jsgcinlines.h
@@ +529,5 @@
> +#ifdef JSGC_GENERATIONAL
> +    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.

Comment 6

[Jan de Mooij [:jandem]]

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

Comment 7

[Terrence Cole [:terrence]]

Attachment: fix_newobjectfromhit_allocation-v1.diff

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.

Comment 8

[Terrence Cole [:terrence]]

https://tbpl.mozilla.org/?tree=Try&rev=aa28de3cb443

Comment 9

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdf5a814023

Comment 10

[Hannes Verschore [:h4writer]]

I see 13% improvement on ss-aes. Bringing ggc on par with non-ggc on this benchmark

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cdf5a814023