Closed Bug 1146662 Opened 5 years ago Closed 5 years ago

Consider refactoring ArenasToUpdate::next to work with a range-based for loop

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

References

Details

Attachments

(1 file, 2 obsolete files)

After bug 1143042 lands, the loop in ArenasToUpdate::next will be the only remaining 'manual' iteration of AllocKinds. It would be nice to switch it over to using AllAllocKinds(), if only for consistency.
ArenasToUpdate::next is clever and kind of scary, with the way it uses goto. This patch rewrites it in a way that I think is somewhat more readable, albeit slightly longer. There are a couple of caveats to the approach:

1) Since it still needs to jump 'into the loop' at the right point *somehow*, I added a parameter to AllAllocKinds (and ObjectAllocKinds) to set the initial AllocKind in the range. This looks a little weird considering the name of the function, but considering the way range-based for loops are written I don't think it'll be confused with an outparam.

2) Simply doing |for (kind : AllAllocKinds(kind))| doesn't work, because the 'kind' before the colon *declares a variable*. Leaving out the type to imply 'auto' is a C++1X extension, but that doesn't help us. Instead I used |for (i : AllAllocKinds(kind))|, and update 'kind' before returning an arena.

Tentatively flagging you for review, Terrence, since you've been reviewing the AllocKind changes, but I believe jonco wrote this code.
Attachment #8582036 - Flags: review?(terrence)
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #1)
> Instead I used |for (i : AllAllocKinds(kind))|

|for (auto i : AllAllocKinds(kind))|, even :)
Comment on attachment 8582036 [details] [diff] [review]
Refactor ArenasToUpdate::next to work with a range-based for loop.

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

Looks fine to me, but I'll defer to Jon for the rest.

::: js/src/gc/Heap.h
@@ +121,4 @@
>  }
>  
>  inline decltype(mozilla::MakeEnumeratedRange<int>(AllocKind::OBJECT0, AllocKind::OBJECT_LIMIT))
> +ObjectAllocKinds(AllocKind initial = AllocKind::OBJECT0)

We're not using the new feature for ObjectAllocKinds, so perhaps instead of adding an arg to All/ObjectAllocKinds, could we instead add a new "SomeAllocKinds(start, limit)"?
Attachment #8582036 - Flags: review?(terrence)
Attachment #8582036 - Flags: review?(jcoppeard)
Attachment #8582036 - Flags: feedback+
Addressed the comment, and added some comments to all these new functions (which should probably land regardless of the fate of this patch).

(In reply to Terrence Cole [:terrence] from comment #3)
> We're not using the new feature for ObjectAllocKinds, so perhaps instead of
> adding an arg to All/ObjectAllocKinds, could we instead add a new
> "SomeAllocKinds(start, limit)"?

Done, though I went with 'first', 'limit' for parity with AllocKind::FIRST, AllocKind::LIMIT ;)
Attachment #8582036 - Attachment is obsolete: true
Attachment #8582036 - Flags: review?(jcoppeard)
Attachment #8582093 - Flags: review?(jcoppeard)
Hmm, try looks green, but it doesn't seem to have triggered a ggc/cgc run (whatever it's called now):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8222382567ff&exclusion_profile=false

I'm not sure how to request one of those, but that's where I would expect this code to be used.
Oh, apparently they're showing up as 'B' builds because of a bug. And I shouldn't have toggled that exclusion_profile stuff. So things are looking green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8222382567ff
Comment on attachment 8582093 [details] [diff] [review]
v2 - Refactor ArenasToUpdate::next to work with a range-based for loop.

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

I think this is less readable than the original because it obscures the fact that this is essentially a nested loop.  I grant that it is less surprising though.  It was more important to have it like this when there was an another for loop for the zones.

If you want to go ahead with this, please remove the comment about "generator-like arrangement" because that will no longer be true.
Attachment #8582093 - Flags: review?(jcoppeard) → review+
Carrying forward r=jonco. Just updated the comment.

(In reply to Jon Coppeard (:jonco) from comment #7)
> I think this is less readable than the original because it obscures the fact
> that this is essentially a nested loop.

Hm, yeah. I tried to think of a way to preserve the original structure, but even pre-initializing the range expression outside the loop, the goto still ends up crossing initialization of the range-based for loop header. It seems like jumping into a range-based for loop is just not possible (jumping *out* of it does work, though).

> I grant that it is less surprising
> though.  It was more important to have it like this when there was an
> another for loop for the zones.

Yeah, I can see this being very concise when even more nested for loops get involved. It did take me a while before I full understood how it worked though (with it jumping to after the return in the inner for loop).

> If you want to go ahead with this, please remove the comment about
> "generator-like arrangement" because that will no longer be true.

Done.
Attachment #8582093 - Attachment is obsolete: true
Attachment #8583908 - Flags: review+
Keywords: checkin-needed
Blocks: 1148214
https://hg.mozilla.org/mozilla-central/rev/176ade0f680d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.