751003 - Consolidate all GC heap structures and methods into a single header

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

GC heap internals are spread across jsgc.h and jscell.h right now, and it's easy to get warnings if things don't match up just so (he said, with the voice of experience).  Move all this stuff into a single header (with minimized dependencies) with declarations and definitions properly interspersed to not produce build warnings.  (The patch depends on bug 750907's patch.)

This is purely code motion; no semantic changes were made.  Not that it'd be possible to see that, of course, in this mess of motion...

We could minimize dependencies even further with a little effort.  Using slightly different macros and moving JS_BITS_PER_WORD into mfbt would let us get rid of the jstypes.h dependency.  Using mfbt assertions would let us get rid of the jsutil.h dependency.  Both ideas either require enough effort -- getting JS_BITS_PER_WORD upstreamed, or providing meaningful explanations for the static assertions -- that I'm going to punt them to later.

Comment 1

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

Comment on attachment 620172 [details] [diff] [review]
Patch

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

::: js/src/gc/Heap.h
@@ +34,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

I guess we should switch to the new license block.

@@ +37,5 @@
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +#ifndef gc_chunk_h___
> +#define gc_chunk_h___

Seems like the name here is wrong.

@@ +134,5 @@
> +    inline bool isAligned() const;
> +#endif
> +};
> +
> +enum State {

I think this should go in jsgc.h. It's unrelated to heap organization.

::: js/src/jsgc.h
@@ -904,2 @@
>  inline Chunk *
>  ArenaHeader::chunk() const

Why did you leave out ArenaHeader::chunk() and ArenaHeader::isEmpty()? I don't like it when we spread classes across header files like this.

@@ -926,2 @@
>  inline ArenaHeader *
>  ArenaHeader::getNextDelayedMarking() const

Same for the delayed marking stuff.

Comment 2

[Matt Brubeck (:mbrubeck)]

Sorry, backed out on inbound because of build failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26738df8a4e0

Comment 3

[Jeff Walden [:Waldo]]

Relanded unmodified (this patch was atop another one that was faulty, and it seemed unwise to disentangle things rather than just back out everything atop that):

https://hg.mozilla.org/integration/mozilla-inbound/rev/d2f386325659

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d2f386325659