1926228 - The code that releases Arenas is convoluted and could use tidying up

Status: NEW

(Core :: JavaScript: GC, task, P2)

Description

[Jon Coppeard (:jonco)]

As pointed out in code review for bug 1925581 the code that releases arenas is complicated and could do with tidying up. Steve had a bunch of ideas to improve this:

Tracing through everything that's happening, it still seems a little convoluted how everything happens. Multiple things are called "release" but kind of mean different things by that. arena->discard or arena->finalize might be better.

But also:

GCRuntime::releaseArena reaches into arena->zone()->gcHeapSize to update its accounting.
then it calls arena->release, which is mostly local stuff except for reaching into GCRuntime::atomMarking to unregister the arena.
then ArenaChunk::releaseArena does a single unit of almost batchable work (only almost because it's per ArenaChunk)
it also does updateChunkListAfterFree which is an operation on ArenaChunk that passes in a GCRuntime* even though almost everything it does is operations on GCRuntime. And the logic looks a little weird because it runs after info.numArenasFree has been updated.

None of this is awful, but this new batching setup does lots of different pieces separately. Is there a better way of organizing this stuff? There might not be.

I'm thinking of things like:

would it be better to split out atom arenas from other arenas more? The code does a lot of branching based on whether something is for atoms or not, and now that includes the locking.
instead of ArenaChunk::updateChunkListAfterFree(gc, numArenasFree, lock), what about GCRuntime::updateChunkListAfterFree(chunk, bool wasFull, lock)?
the ArenaChunk manipulation could be batched, and then the lock grabbed once per distinct ArenaChunk:

for (size_t end = 0; end < count; ) {
  Arena* first = arenasToRelease[end++];
  for (size_t i = end; i < count; i++) {
    if (arenasToRelease[i]->chunk() == first->chunk()) {
      std::swap(arenasToRelease[i], arenasToRelease[end++]);
    }
  }
  AutoGCLock lock(this);
  first->chunk()->releaseArenas(this, first, end, lock);
}