Last Comment Bug 751003 - Consolidate all GC heap structures and methods into a single header
: Consolidate all GC heap structures and methods into a single header
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 750907
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-01 20:04 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-05-04 10:51 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (67.20 KB, patch)
2012-05-01 20:04 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
wmccloskey: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-01 20:04:24 PDT
Created attachment 620172 [details] [diff] [review]
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 (:billm) 2012-05-02 11:02:05 PDT
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) 2012-05-03 14:58:11 PDT
Sorry, backed out on inbound because of build failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26738df8a4e0
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-05-03 16:49:42 PDT
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] 2012-05-04 10:51:59 PDT
https://hg.mozilla.org/mozilla-central/rev/d2f386325659

Note You need to log in before you can comment on or make changes to this bug.