Tidy up GC zeal sweep actions

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P3
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

61 Branch
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

7 months ago
It's bothered me for a while the way the MaybeYield() function to create a sweep action that checks the zeal state and possilby yields to the mutator takes both the zeal mode and an action to wrap.  This was really just a hack because it needed to know the appropriate template arguments for the sweep action, which it can get from the wrapped action.  But it makes things more confusing.
Assignee

Updated

7 months ago
Priority: -- → P3
Assignee

Comment 1

7 months ago
The patch removes the action parameter when creating a MaybeYield action and this means we have to explicitly specify the template arguments.  So now there is MaybeYield() and MaybeYieldInZoneLoop().  The template arguments are the arguments passed to the action when it is run and we need the extra Zone* argument for the latter.  Removing the action parameter makes GCRuntime::initSweepActions() more straightforward I think.

Previously the MaybeYield action was not built unless GC zeal was configured, but now we create these actions and drop them if there's no GC zeal.  This works by adding a canSkip() method to sweep actions which which returns true for these actions if GC zeal is not configured.  They are then are filtered out in SweepActionSequence::init().

Finally I moved the extra logic about IncrementalMultipleSlices zeal only happening once at the start of sweeping into GCRuntime::shouldYieldForZeal().
Attachment #9020834 - Flags: review?(sphink)
Comment on attachment 9020834 [details] [diff] [review]
bug1502940-tidy-zeal-actions

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

::: js/src/gc/GC.cpp
@@ +6941,5 @@
> +                MaybeYield(ZealMode::IncrementalMultipleSlices),
> +                MaybeYield(ZealMode::YieldBeforeSweepingAtoms),
> +                Call(&GCRuntime::sweepAtomsTable),
> +                MaybeYield(ZealMode::YieldBeforeSweepingCaches),
> +                Call(&GCRuntime::sweepWeakCaches),

You're right, it's nicer to unbundle the yielding from the action.

::: js/src/gc/GCRuntime.h
@@ +63,5 @@
>  {
>      virtual ~SweepAction() {}
>      virtual IncrementalProgress run(Args... args) = 0;
>      virtual void assertFinished() const = 0;
> +    virtual bool canSkip() { return false; }

'canSkip' suggests a possibility to me, not a certainty. How do you feel about 'skip()' or 'shouldSkip()'?
Attachment #9020834 - Flags: review?(sphink) → review+

Comment 3

7 months ago
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/131bc0e56197
Tidy up sweep actions that implement GC zeal modes r=sfink

Comment 4

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/131bc0e56197
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.