Add tunables and environment variables for pretenuring

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: pbone, Assigned: pbone)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Assignee)

Description

6 months ago
Add an environment variable or maybe tunable parameter to enable and disable pretenuring.  It can be useful to disable this to test a hyphothesis.
(Assignee)

Updated

6 months ago
Priority: -- → P3
(Assignee)

Updated

6 months ago
Assignee: nobody → pbone
Status: NEW → ASSIGNED
(Assignee)

Updated

5 months ago
Blocks: 1510560
(Assignee)

Comment 2

5 months ago
This function helps the nursery find the GCSchedulingTunables structure.
Attachment #9030986 - Flags: review?(jcoppeard)
(Assignee)

Comment 3

5 months ago
Attachment #9030987 - Flags: review?(jcoppeard)
(Assignee)

Comment 4

5 months ago
Attachment #9030988 - Flags: review?(jcoppeard)
(Assignee)

Updated

5 months ago
Attachment #9030978 - Attachment description: Set pretenuring from an environment variable → Bug 1507377 - Set pretenuring from an environment variable
Attachment #9030978 - Flags: review?(jcoppeard)
(Assignee)

Updated

5 months ago
Attachment #9030978 - Attachment is obsolete: true
Attachment #9030978 - Flags: review?(jcoppeard)
(Assignee)

Updated

5 months ago
Summary: Add a JSGC_DISABLE_PRETENURING environment variable / tunable → Add tunables and environment variables for pretenuring
Comment on attachment 9030986 [details] [diff] [review]
Patch 1507377 - Add a Nursery::tunables() function

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

::: js/src/gc/Nursery.cpp
@@ +1259,4 @@
>    return runtime()->gc.stats();
>  }
>  
> +js::gc::GCSchedulingTunables& js::Nursery::tunables() const {

Please make simple accessors like this inline.

::: js/src/gc/Nursery.h
@@ +549,4 @@
>    JSRuntime* runtime() const { return runtime_; }
>    gcstats::Statistics& stats() const;
>  
> +  js::gc::GCSchedulingTunables& tunables() const;

Probably should return a const& if you don't need to change the tunables in nursery code.
Attachment #9030986 - Flags: review?(jcoppeard) → review+
Comment on attachment 9030987 [details] [diff] [review]
Patch 1507377 - Add tunables for pretenure thresholds

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

::: js/src/gc/GC.cpp
@@ +1476,5 @@
>        nurseryFreeThresholdForIdleCollection_ = value;
>        break;
> +    case JSGC_PRETENURE_THRESHOLD: {
> +      float newThreshold = value / 100.0f;
> +      if (newThreshold <= 0.0f || newThreshold >= 1.0f) {

We don't allow this to be set to zero or one?

::: js/src/gc/Scheduling.h
@@ +435,5 @@
>  
> +  /*
> +   * JSGC_PRETENURE_THRESHOLD
> +   *
> +   * Fraction of objects tenured to trigger pretenuring (between 0 and 1).  If

nit: single space after period.

@@ +444,5 @@
> +
> +  /*
> +   * JSGC_PRETENURE_GROUP_THRESHOLD
> +   *
> +   * Number of objects in a group are tenured in a single nursery collection

There's something missing... how about "number of objects in a group that can be tenured ... before the group is pretenured"
Attachment #9030987 - Flags: review?(jcoppeard) → review+
Comment on attachment 9030988 [details] [diff] [review]
Patch 1507377 - If the pretenure rate is zero, disable pretenuring

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

How about we make setting the threshold to one disable pretenuring?  This is consistent with the meaning of the threshold and doesn't need any extra checks.
Attachment #9030988 - Flags: review?(jcoppeard)
Comment on attachment 9031011 [details] [diff] [review]
Patch 1507377 - Set pretenuring from an environment variable

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

This seems fine.

I'm wary of creating environment variables for all our tunables though.
Attachment #9031011 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 10

5 months ago
(In reply to Jon Coppeard (:jonco) from comment #7)
> Comment on attachment 9030987 [details] [diff] [review]
> Patch 1507377 - Add tunables for pretenure thresholds
> 
> Review of attachment 9030987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/gc/GC.cpp
> @@ +1476,5 @@
> >        nurseryFreeThresholdForIdleCollection_ = value;
> >        break;
> > +    case JSGC_PRETENURE_THRESHOLD: {
> > +      float newThreshold = value / 100.0f;
> > +      if (newThreshold <= 0.0f || newThreshold >= 1.0f) {
> 
> We don't allow this to be set to zero or one.

Neither made sense to me. Until we want to support disabling it (next patch).

> @@ +444,5 @@
> > +
> > +  /*
> > +   * JSGC_PRETENURE_GROUP_THRESHOLD
> > +   *
> > +   * Number of objects in a group are tenured in a single nursery collection
> 
> There's something missing... how about "number of objects in a group that
> can be tenured ... before the group is pretenured"

It was still unclear, I've rewritten it:

    During a single nursery collection, if this many objects from the same
    object group are tenured, then that group will be pretenured.
(Assignee)

Comment 11

5 months ago
(In reply to Jon Coppeard (:jonco) from comment #9)
> Comment on attachment 9031011 [details] [diff] [review]
> Patch 1507377 - Set pretenuring from an environment variable
> 
> Review of attachment 9031011 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This seems fine.
> 
> I'm wary of creating environment variables for all our tunables though.

I filed that idea as a seperate bug anyway, since I wasn't sure.
(Assignee)

Comment 12

5 months ago
I think you're right about a threshold of 1.0 being used to disable pretenuring.  I've written this, it does still use an extra check because I want it to also disable pretenruing when the collection is because of a full storebuffer.

I've merged this with the previous patch.  It's just simplier to read them togeather.
Attachment #9030987 - Attachment is obsolete: true
Attachment #9030988 - Attachment is obsolete: true
Attachment #9031314 - Flags: review?(jcoppeard)
Comment on attachment 9031314 [details] [diff] [review]
Bug 1507377 - Add tunables for pretenure thresholds

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

Great, thanks.
Attachment #9031314 - Flags: review?(jcoppeard) → review+

Comment 14

5 months ago
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0bf46f5bb7
Add a Nursery::tunables() function r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3a6aa5f0ca6
Add tunables for pretenure thresholds r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/3419d7dc0a29
Set pretenuring from an environment variable r=jonco

Comment 15

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3c0bf46f5bb7
https://hg.mozilla.org/mozilla-central/rev/f3a6aa5f0ca6
https://hg.mozilla.org/mozilla-central/rev/3419d7dc0a29
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.