As mentioned in bug 1259347, at the moment we pre-reserve 16MiB of address space for the nursery. Instead, we could allocate nursery chunks as we need them. This can re-use the current pool of empty chunks that we use for allocating arenas.

There are a couple of wrinkles with this: - our JITs currently use the start and end address of the nursery to check if a pointer is inside - changing the nursery size (and hence disabling/enabling the nursery) become fallible - the Nursery::isInside that operates on any pointer will become linear in the number of nursery chunks I tested changing the JITs to use the location word in the chunk trailer and didn't see a significant performance impact on octane.

Refactor chunk location word to use an enum class.

Change our JITs to check whether a pointer is inside the nursery by looking at the chunk location rather than baking in the address of the nursery region. I'll ask jandem to review later pending terrence's review on the other patches.

Tidyup to move ChunkTrailer out of ChunkInfo so it's the last thing in Chunk.

Split out separate methods for allocating/recycling chunks.

Switch over the nursery to using dynamically allocated chunks.

Comment on attachment 8778292 [details] [diff] [review] bug1291292-refactor-chunk-location Review of attachment 8778292 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this! Back when Lars added it for PJS, it was actually used as a semi-int in a few places in PJS. Now that the usage is simpler it makes much more sense as an enum.

Comment on attachment 8778296 [details] [diff] [review] bug1291292-refactor-chunk-trailer Review of attachment 8778296 [details] [diff] [review]: ----------------------------------------------------------------- Woot! I'm pretty sure I have a patch that looks exactly like this in one of my backlogged queues somewhere.

Octane results with these patches applied showed a slowdown of 1.3% from 31231.1 to 30815.6 on an average of 10 runs, which is within the usual noise threshold.

Comment on attachment 8778297 [details] [diff] [review] bug1291292-refactor-chunk-alloc Review of attachment 8778297 [details] [diff] [review]: ----------------------------------------------------------------- Yup, I see where you're going with this.

Comment on attachment 8778298 [details] [diff] [review] bug1291292-dynamically-alloc-nursery-chunks Review of attachment 8778298 [details] [diff] [review]: ----------------------------------------------------------------- You made fast work of this! As to the regression, I'd try running with JS_GC_PROFILE_NURSERY=0 and sum up the resize columns to see if the OS is having trouble keeping up with our allocation rate. This seems to be a problem particularly in MacOS, so that's the first place I'd look. We probably need to update the number of chucks that background allocation can provide to keep up with the new demand, since I think it's extremely low, currently. ::: js/src/gc/Nursery.h @@ +235,3 @@ > > // Free space remaining, not counting chunk trailers. > MOZ_ALWAYS_INLINE size_t approxFreeSpace() const { This is now exactFreeSpace. Or just freeSpace().

Comment on attachment 8778295 [details] [diff] [review] bug1291292-use-chunk-location-in-jit Requesting review for JIT changes to detect nursery pointers by checking the location value stored in the chunk trailer rather than checking against a single address range.

Comment on attachment 8778295 [details] [diff] [review] bug1291292-use-chunk-location-in-jit Review of attachment 8778295 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comment below addressed. ::: js/src/jit/x64/MacroAssembler-x64.cpp @@ +538,5 @@ > MOZ_ASSERT(ptr != temp); > MOZ_ASSERT(ptr != scratch); > > + Label done; > + branchTestPtr(Assembler::Zero, ptr, ptr, cond == Assembler::Equal ? &done : label); I'd expect all pointers here to be non-null. If there is a place where we can pass a null JSObject* (I don't see it in this patch, but didn't check closely), can we move this check to the caller?

(In reply to Jan de Mooij [:jandem] from comment #13) Thanks, it would be great to remove this check if possible. It turns out that the pointer can be null if this is called from CodeGenerator::visitPostWriteBarrierCommonO. This is because LIRGenerator::visitPostWriteBarrier and visitPostWriteElementBarrier emit LPostWriteBarrierO or LPostWriteElementBarrierO if the value's MIRType is Object or ObjectOrNull. I moved the check to visitPostWriteBarrierCommonO and everything looks good.

Pushed by jcoppeard@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/61d8000fc0a9 Use an enum class for the chunk location values r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/dea9c5788c50 Use chunk location word for nursery test in JIT code r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ef6e3777b5 Refactoring to move ChunkTrailer out of ChunkInfo r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/0987e46667b2 Split out separate methods for allocating / recycling chunks r=terrence https://hg.mozilla.org/integration/mozilla-inbound/rev/1a4509a3e2ce Use dynamic chunk allocation for the nursery r=terrence

https://hg.mozilla.org/mozilla-central/rev/61d8000fc0a9 https://hg.mozilla.org/mozilla-central/rev/dea9c5788c50 https://hg.mozilla.org/mozilla-central/rev/e6ef6e3777b5 https://hg.mozilla.org/mozilla-central/rev/0987e46667b2 https://hg.mozilla.org/mozilla-central/rev/1a4509a3e2ce

This bug regressed allocation-related octane benchmarks, like raytrace, regexp, etc, by ~10%.

I can reproduce the regression on both my linux64 laptop and desktop. e.g., m-c 309403:054d4856cea6 RayTrace: 98049 ---- Score (version 9): 98049 m-c 309160:0987e46667b2 (309161 is the offending commit) RayTrace: 110998 ---- Score (version 9): 110998

Would this change explain a sudden change[1] in the telemetry measure GC_NURSERY_BYTES? It raised an alert[2] whose changelog[3] contains reference to this bug. There was a corresponding sudden change[4] and alert[5] for GC_MINOR_US. Would this be related? [1]: https://mzl.la/2bfLlOG [2]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1931/alerts/?from=2016-08-13&to=2016-08-13 [3]: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=233ab21b64b5d5e9f2f16ea2d4cfb4c8b293c5c4&tochange=2ed7e61b988d2466a61528f66050596ef272ebda [4]: https://mzl.la/2bfNCZQ [5]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1305/alerts/?from=2016-08-13&to=2016-08-13

The GC_NURSERY_BYTES changes is more likely to be due to bug 1293239. I've filed bug 1296272 to track the regressions.

(In reply to Shu-yu Guo [:shu] from comment #18) Can I check whether you disable poisoning by setting JSGC_DISABLE_POISONING=1 when running these tests? (This is disabled in release channels but enabled on nightly for testing purposes). Here are my before and after results for this changeset on linux64, with and without poisoning disabled (average of ten runs): Pre, poisoning disabled: RayTrace: 125901.6 Pre, without poisoning disabled: RayTrace: 110998.0 Post, poisoning disabled: RayTrace: 127233.6 Post, without poisoning disabled: RayTrace: 97723.4

(In reply to Jon Coppeard (:jonco) from comment #22) > (In reply to Shu-yu Guo [:shu] from comment #18) > Can I check whether you disable poisoning by setting > JSGC_DISABLE_POISONING=1 when running these tests? (This is disabled in > release channels but enabled on nightly for testing purposes). > > Here are my before and after results for this changeset on linux64, with and > without poisoning disabled (average of ten runs): > > Pre, poisoning disabled: > RayTrace: 125901.6 > Pre, without poisoning disabled: > RayTrace: 110998.0 > Post, poisoning disabled: > RayTrace: 127233.6 > Post, without poisoning disabled: > RayTrace: 97723.4 Ooh, I didn't know about that environment variable when benchmarking. With JSGC_DISABLE_POISONING=1, the regression I saw goes away. Thanks, re-closing the bug.