Closed Bug 1437602 Opened 2 years ago Closed 2 years ago

Refactor gc/Zone.h a little

Categories

(Core :: JavaScript: GC, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch zone-1-split-out-iterators (obsolete) — Splinter Review
Patch to split out iterators from Zone.h.  Previously this contained the iterators for zone groups, zones and compartments.  This moves them to a new gc/Iteration.h header.

I renamed the existing gc/Iteration-inl.h header to gc/GCIteration.h.  This file contains internal iterators used mainly from the GC itself.

It turned out that the AutoPrepareForTracing constructor took a ZoneSelector argument that it didn't use so I removed that.
Attachment #8950284 - Flags: review?(pbone)
This patch moves all the scheduling-related structures to a new header, gc/Scheduling.h.  I also moved the huge comment describing this, but I didn't check it's still correct... this could do with a thorough review.
Attachment #8950286 - Flags: review?(pbone)
Priority: -- → P3
Comment on attachment 8950284 [details] [diff] [review]
zone-1-split-out-iterators

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

I didn't check for changes to the moved code itself.

If there are no changes to the ieterators themselves then this is r+ except for two minor issues.

Thanks.

::: js/src/shell/js.cpp
@@ +79,5 @@
>  #endif // defined(JS_BUILD_BINAST)
>  
>  #include "frontend/Parser.h"
> +#include "gc/Iteration.h"
> +//#include "gc/GCInternals.h"

Should this commented out line be deleted?

::: js/src/vm/SelfHosting.cpp
@@ +39,5 @@
>  #include "builtin/SIMD.h"
>  #include "builtin/Stream.h"
>  #include "builtin/TypedObject.h"
>  #include "builtin/WeakMapObject.h"
> +//#include "gc/GCIteration.h"

And here.
Attachment #8950284 - Flags: review?(pbone) → review+
Comment on attachment 8950286 [details] [diff] [review]
zone-2-split-out-scheduling

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

Again, I'm assuming there's no change to the moved code itself.

Thanks for doing this r+.
Attachment #8950286 - Flags: review?(pbone) → review+
(In reply to Paul Bone [:pbone] from comment #2)
> Should this commented out line be deleted?

Ugh, yes, I forgot to take those out.

Cheers!
I had to rename GCIterators.h to GCIterators-inl.h because our style checker complained about it including jsgcincludes.h.  Carrying r+.
Attachment #8950284 - Attachment is obsolete: true
Attachment #8951254 - Flags: review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40bbf952db46
Split out zone and compartment iterators from gc/Zone.h r=pbone
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a7cc519c961
Move all scheduling related data structures to a new gc/Scheduling.h r=pbone
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a061e3873df0
Fix bustage due to missing explicit keyword r=me on a CLOSED TREE
You need to log in before you can comment on or make changes to this bug.