Closed Bug 751003 Opened 10 years ago Closed 10 years ago

Consolidate all GC heap structures and methods into a single header


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)




(1 file)

Attached patch PatchSplinter Review
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.
Attachment #620172 - Flags: review?(wmccloskey)
Comment on attachment 620172 [details] [diff] [review]

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.
Attachment #620172 - Flags: review?(wmccloskey) → review+
Sorry, backed out on inbound because of build failure:
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):
Target Milestone: --- → mozilla15
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.