936583 - Move common fields of the chunk footer to a common struct

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: chunk_info_common-v0.diff

This moves the shared JSRuntime* at the end of all 1MiB chunks, both Nursery and Tenured, into the new struct: ChunkInfoCommon. This should make the shared nature of the layout much more obvious.

I also converted everything over to static_assert now that we can use it directly and, in particular, added a new static assertion that NumArenasPerChunk == 252. We constantly worry about accidentally overflowing our padding and losing arenas so we might as well just assert it.

And a try run to verify that the number above is actually correct on all platforms.
https://tbpl.mozilla.org/?tree=Try&rev=fa5e540a9a39

Comment 1

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

Comment on attachment 829421 [details] [diff] [review]
chunk_info_common-v0.diff

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

Looks good, although we may have to stick with JS_ARRAY_LENGTH for Windows.

::: js/src/gc/Heap.h
@@ -453,5 @@
> -        /*
> -         * auxNextLink packing assumes that ArenaShift has enough bits
> -         * to cover allocKind and hasDelayedMarking.
> -         */
> -        JS_STATIC_ASSERT(ArenaShift >= 8 + 1 + 1 + 1);

Please leave this one here, but not wrapped in a method. It makes more sense here, right after the fields it's referencing.

@@ -471,5 @@
>          JS_ASSERT(!allocatedDuringIncremental);
>          JS_ASSERT(!hasDelayedMarking);
>          zone = zoneArg;
>  
> -        JS_STATIC_ASSERT(FINALIZE_LIMIT <= 255);

Please just convert this one to a static_assert and don't move it down. I think it belongs here.

@@ +597,5 @@
> + * The tail of the chunk info is shared between all chunks in the system, both
> + * nursery and tenured. This structure is locatable from any GC pointer by
> + * aligning to 1MiB.
> + */
> +struct ChunkInfoCommon

What if we call this ChunkTrailer?

@@ +636,5 @@
>      /* Number of GC cycles this chunk has survived. */
>      uint32_t        age;
>  
> +    /* Information shared by all Chunk types. */
> +    ChunkInfoCommon common;

This could be called trailer.

::: js/src/jsgc.cpp
@@ +275,5 @@
>  }
>  #endif
>  
>  /* static */ void
>  Arena::staticAsserts()

No need to put these in a method any more.

Comment 2

[Terrence Cole [:terrence]]

(In reply to Bill McCloskey (:billm) from comment #1)
> ::: js/src/jsgc.cpp
> @@ +275,5 @@
> >  }
> >  #endif
> >  
> >  /* static */ void
> >  Arena::staticAsserts()
> 
> No need to put these in a method any more.

Actually, Arena::ThingSizes and Arena::FirstThingOffsets are a bit troublesome. They are declared private in Arena, but defined in jsgc.cpp. If we put the assertions next to the declarations in gc/Heap.h, we get an error because they are undefined. If we put the assertions at top level next to the definitions in jsgc.cpp, we error because they are private. I'm going to check in keeping the staticAsserts method because I think that's still cleaner than making the fields public. I'll gladly push a follow up if you know of a better way to do this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/487a2bf16ae5

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/487a2bf16ae5