1004790 - Split off a new class FreeList from FreeSpan

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

FreeSpan is really two types -- some of the operations are only relevant if
it's a freeLists[] value, and some of the operations are only relevant if it's
not. So we should split them.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Split off a new class FreeList from FreeSpan.

This patch creates a new class, FreeList, which inherits from FreeSpan. It
moves several of the methods from FreeSpan to FreeList. The moved methods have
the following minimal changes:
- allocate/infallibleAllocate use the new setHead() method.
- isSameNonEmptySpan() now takes a reference instead of a pointer, and the
  callers had their (this,argument) pair switched.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Make FreeList contain a FreeSpan instead of being a subclass

This patch changes FreeList from "is a FreeSpan" to "has a FreeSpan". This
makes clearer the fact that there isn't that much overlap between the two 
classes -- the only methods present in both classes are isEmpty() and
arenaAddress(). It also allows FreeList's data to be encapsulated (i.e.
private).

The patch also adds some extra assertions, and does other minor clean-ups, such
as removing initAsEmpty's never-used optional parameter.

To make sure that there was no remaining code that cast FreeList to FreeSpan or
vice versa (or anything else like that) I temporarily added some extra fields
before FreeList::head, and this flushed out a couple of places I'd missed.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8416195 [details] [diff] [review]
(part 1) - Split off a new class FreeList from FreeSpan.

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

::: js/src/gc/Heap.h
@@ +388,5 @@
> +     * initialization so to allocate we simply return the first thing in the
> +     * arena and set the free list to point to the second.
> +     */
> +    MOZ_ALWAYS_INLINE void *allocateFromNewArena(uintptr_t arenaAddr, size_t firstThingOffset,
> +                                                size_t thingSize) {

Can you indent this line over one more? It's broken from before.

Comment 4

[Nicholas Nethercote [inactive]]

> Can you indent this line over one more? It's broken from before.

I did that in the second patch.

Try server looks good:
https://tbpl.mozilla.org/?tree=Try&rev=ae49f493005a

Comment 5

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21332f87ca19
https://hg.mozilla.org/integration/mozilla-inbound/rev/41ae09162dd7

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21332f87ca19
https://hg.mozilla.org/mozilla-central/rev/41ae09162dd7