1507377 - Add tunables and environment variables for pretenuring

Status: RESOLVED FIXED

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

Description

[Paul Bone [:pbone]]

Add an environment variable or maybe tunable parameter to enable and disable pretenuring.  It can be useful to disable this to test a hyphothesis.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1507377 - Set pretenuring from an environment variable

Comment 2

[Paul Bone [:pbone]]

Attachment: Patch 1507377 - Add a Nursery::tunables() function

This function helps the nursery find the GCSchedulingTunables structure.

Comment 3

[Paul Bone [:pbone]]

Attachment: Patch 1507377 - Add tunables for pretenure thresholds

Comment 4

[Paul Bone [:pbone]]

Attachment: Patch 1507377 - If the pretenure rate is zero, disable pretenuring

Comment 5

[Paul Bone [:pbone]]

Attachment: Patch 1507377 - Set pretenuring from an environment variable

Comment 6

[Jon Coppeard (:jonco)]

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.

Comment 7

[Jon Coppeard (:jonco)]

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"

Comment 8

[Jon Coppeard (:jonco)]

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.

Comment 9

[Jon Coppeard (:jonco)]

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.

Comment 10

[Paul Bone [:pbone]]

(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.

Comment 11

[Paul Bone [:pbone]]

(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.

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1507377 - Add tunables for pretenure thresholds

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.

Comment 13

[Jon Coppeard (:jonco)]

Comment on attachment 9031314 [details] [diff] [review]
Bug 1507377 - Add tunables for pretenure thresholds

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

Great, thanks.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3c0bf46f5bb7
https://hg.mozilla.org/mozilla-central/rev/f3a6aa5f0ca6
https://hg.mozilla.org/mozilla-central/rev/3419d7dc0a29