Separating this from bug 1250634 since that's a pretty involved change and I don't want to drag it out. I have some more ideas for how to simplify this; in particular, it would be nice to merge ArenaHeader into Arena, since the separation serves no real purpose. This does require some prep work however, even after bug 1250634.

Note to self: Remember to move ArenaCellIterImpl's init() into the constructor.

This patch doesn't really make anything better on its own, but it lays the last groundwork needed to get rid of ArenaHeader. It moves allocation into FreeSpan and moves it up to the top of ArenaHeader so we no longer have to worry about offsets (and can just use |this| with a few debug asserts). Our placeholder is now a FreeSpan instead of an ArenaHeader ;)

Moving init() into the constructor didn't work out, because we actually can't always know what arena to use during construction. Instead I cleaned up all the iterators a bit, allowing you the consumer to choose between initializing during construction, or initializing later. However, nothing actually uses this - so aside from a bit of a cleanup, and making the assertions more consistent, this patch does nothing but make things slightly more complicated. I think it's still barely an improvement, but I'll let you make the call.

And the main patch for this bug. Sorry about the size, I thought too late about a way to split it up a little (but it wouldn't have improved it much). This is almost all code motion, s/ArenaHeader/Arena/ and s/aheader/arena/, but there are a few places where we previously accessed ArenaHeaders through Arenas, which is no longer needed. There should be nothing that changes behavior, but I touched up a few functions and extended a few comments (rewrapped a few others). With this, we only have 2 classes involved in Arena allocation: Arena and FreeSpan. I think this is optimal: reducing it to just Arena would lead to a lot of open coding for the offsets, and I can't think of any step that would be simplified by adding a 3rd class. The only wrinkle is that with no ArenaHeader class, the header size is less well defined; I added a constant (ArenaHeaderSize - they're still header fields of Arena, after all) to HeapAPI.h to get around this.

The unused variable warning bites me again. I'll just reupload these while no one is looking :P

Removes an ill-conceived change from part 2 that introduced 4 hazards.

Comment on attachment 8724502 [details] [diff] [review] Part 1: Move allocation into FreeSpan and move firstFreeSpan to the top of Arenas. Review of attachment 8724502 [details] [diff] [review]: ----------------------------------------------------------------- Wow, this is starting to actually read like an allocator! ::: js/src/jsgc.h @@ +593,5 @@ > * update the arena header after the initial allocation. When starting the > * GC we only move the head of the of the list of spans back to the arena > * only for the arena that was not fully allocated. > */ > + AllAllocKindArray<FreeSpan*> freeLists; \o/

Comment on attachment 8724537 [details] [diff] [review] Part 2: Clean up the various iterators a bit. Review of attachment 8724537 [details] [diff] [review]: ----------------------------------------------------------------- This is a huge improvement, ship it.

Comment on attachment 8724503 [details] [diff] [review] Part 3: Merge ArenaHeader into Arena. Review of attachment 8724503 [details] [diff] [review]: ----------------------------------------------------------------- This is even more awesome than I thought it would be. ::: js/src/gc/GCRuntime.h @@ +1176,5 @@ > > /* > * List head of arenas allocated during the sweep phase. > */ > + js::gc::Arena* arenasAllocatedDuringSweep; I think also remove the js::gc:: while you're here as it's not used elsewhere in the class. I'd guess it is from the days when all this was directly on the JSRuntime. ::: js/src/gc/Heap.h @@ +416,5 @@ > + * +---------------+---------+----+----+-----+----+ > + * | header fields | padding | T0 | T1 | ... | Tn | > + * +---------------+---------+----+----+-----+----+ > + * <-------------------------> = first thing offset > + */ Great! It's much more effective to have this right at the top. @@ +428,5 @@ > + * The first span of free things in the arena. Most of these spans are > + * stored as offsets in free regions of the data array, and most operations > + * on FreeSpans take an Arena pointer for safety. However, the FreeSpans > + * used for allocation are stored here, at the start of an Arena, and use > + * their own address to grab the next span within the same Arena. \o/ Full of win! ::: js/src/gc/Marking.cpp @@ +1942,2 @@ > { > + DispatchTraceKindTyped(PushArenaFunctor(), Remove the whitespace at the end of this line.

Carrying forward r+ - updated to match the latest patch in bug 1250634.

Carrying forward r+ - updated to match the latest patch in bug 1250634.

Carrying forward r+ - updated to match the latest patch in bug 1250634. Thanks for the fast reviews! (In reply to Terrence Cole [:terrence] from comment #11) > > + js::gc::Arena* arenasAllocatedDuringSweep; > > I think also remove the js::gc:: while you're here as it's not used > elsewhere in the class. I'd guess it is from the days when all this was > directly on the JSRuntime. Bonus patch incoming. > ::: js/src/gc/Marking.cpp > @@ +1942,2 @@ > > { > > + DispatchTraceKindTyped(PushArenaFunctor(), > > Remove the whitespace at the end of this line. Always one. Fixed!

As discussed on IRC. This compiles for me locally, but I'll send it off to try to make sure it doesn't magically break the build somewhere.

Comment on attachment 8724867 [details] [diff] [review] Part 4: Remove some unneeded qualification from GCRuntime and friends. Er, hold off, this broke everything.

js::GCHelperState needed the qualification to be marked as a friend class. MSVC didn't care. Hopefully that's all it needs: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62684449db41

Comment on attachment 8724882 [details] [diff] [review] Part 4 v2: Remove some unneeded qualification from GCRuntime and friends. Review of attachment 8724882 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c59c0fc07fb https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef36969401d https://hg.mozilla.org/integration/mozilla-inbound/rev/064b832e49db https://hg.mozilla.org/integration/mozilla-inbound/rev/90e6fc731368

https://hg.mozilla.org/mozilla-central/rev/0c59c0fc07fb https://hg.mozilla.org/mozilla-central/rev/7ef36969401d https://hg.mozilla.org/mozilla-central/rev/064b832e49db https://hg.mozilla.org/mozilla-central/rev/90e6fc731368