Closed Bug 1380768 Opened 2 years ago Closed 2 years ago

Add preference for setting maxNurseryBytes

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: sfink, Assigned: pbone)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

It would be useful for experimenting with raising the 16MB limit. Though I'm not sure if it should be in bytes or chunks.
Blocks: 1371162
Assignee: nobody → pbone
Status: NEW → ASSIGNED
Blocks: 1376861
This first patch just fixes some comments.
Attachment #8888650 - Flags: review?(sphink)
This change adds the new preference and allows the nursery to change its maximum size during runtime (at the end of each collection).
Attachment #8888651 - Flags: review?(sphink)
Comment on attachment 8888650 [details] [diff] [review]
(part 1) - Update comments to reflect API changes

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

Thanks for this! I didn't even know JS_NewRuntime was dead.
Attachment #8888650 - Flags: review?(sphink) → review+
Comment on attachment 8888651 [details] [diff] [review]
(part 2) - Add a pref for nursery size, r=sfink.

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

::: dom/base/nsJSEnvironment.cpp
@@ +2608,5 @@
> +{
> +  int32_t pref = Preferences::GetInt(aPrefName, -1);
> +  // handle overflow and negative pref values
> +  uint32_t max = (pref <= 0 || pref >= 0x400000) ? -1 : (uint32_t)pref * 1024;
> +  SetGCParameter(JSGC_MAX_NURSERY_BYTES, max);

Why the random limit of 400000? I guess it's low enough to prevent overflow. But then why that limit specifically, rather than 2GB/1024?

Actually, I think it would probably be more clear to use mozilla::CheckedInt<uint32_t>:

  mozilla::CheckedInt<int32_t> pref(Preferences::GetInt(aPrefName, -1));
  mozilla::CheckedInt<uint32_t> max(pref);
  max *= 1024;
  SetGCParameter(JSGC_MAX_NURSERY_BYTES, max.isValid() ? max.value() : -1);

I'm not sure if that's the clearest way to go about it, but that's one option that comes to mind. The main point is to use CheckedInt's isValid() to handle overflow checking. (I also borrowed it for the negative values there, but I'm not sure whether that makes it more clear or less what's going on.)

::: js/src/gc/Nursery.cpp
@@ +942,5 @@
> +            if (shrinkAllocableSpace(extraChunks)) {
> +                /*
> +                 * Only update the maximum if we succesfully shrank the
> +                 * nursery.  This way we will try this code again if this was
> +                 * unsuccesful.  This can happen when Zeal mode is used.

"successfully", "unsuccessful"

Given that the only way that this will fail is for the zeal mode, and the zeal mode is unlikely to change during a run, isn't this almost guaranteed to just retry over and over again, and from that point on it will never growAllocableSpace() or shrinkAllocableSpace()? It seems like if zeal mode is GenerationalGC, then shrinking the max nursery size is equivalent to permanently disabling nursery resizing. Which, honestly, probably isn't a big deal at all; GenerationalGC looks like a pretty bizarre mode that wants to control the timing of nursery collections tightly (and makes sure to use different parts of the nursery or something, so that stale nursery references are more likely to crash from touching freed nursery memory?)

But if that's the case, I'd prefer that to be more up-front and explicit: at the beginning of maybeResizeNursery, check the zeal mode and return immediately if set.

@@ +949,5 @@
> +            }
> +            previousPromotionRate_ = promotionRate;
> +            return;
> +        } else
> +            maxNurseryChunks_ = newMaxNurseryChunks;

Two coding standards things: when you have an if body that extends over more than one line, you need to {brace} the body *and the body of any else*.

  if (...) {
      two;
      lines;
  } else {
      one line;
  }

Second, we avoid using else when the if ends in an early return.

    if (...) {
        stuff;
        return;
    }
    do_the_other_thing();

::: modules/libpref/init/all.js
@@ +1429,4 @@
>  // Comment 32 and Bug 613551.
>  pref("javascript.options.mem.high_water_mark", 128);
>  pref("javascript.options.mem.max", -1);
> +#if defined(ANDROID) || defined(MOZ_B2G) || defined(XP_IOS)

Huh. I thought we'd removed all b2g references in the code, but it looks like we haven't.

But I don't think this should be here if this is duplicating logic in HeapAPI.h. I think you're better off setting it to -1. Less chance of things getting out of sync. (And less encouragement for people to muck with this value, since I *think* this probably makes it appear in about:config by default?)
Attachment #8888651 - Flags: review?(sphink)
The limit is 4GB, it's measured in KB so it looks arbitrary.  I'll use CheckedInt though, I think that will be clearer, at least it'll be clear that I've chosen the limits of the integer type.  (I copied a pattern in code nearby, so I'll revise it also.)

Yes, I see the preference in about:config.  I didn't realize that there were yet more preferences that wern't shown there.  But I guess by omitting this from all.js it will now be hidden unless someone sets it.  Is there a standard place to document the preference name?  I'll put a note about it in GCSchedulingTunables which is where I'd like to concentrate all the tunables within the SpiderMonkey code.  We should eventually do this with all the others.

Thanks.
Revised patch

In addition to the review feedback I added code to disable the nursery if this preference is set to zero. Without this Firefox crashes when the nursery is configured to be zero-sized but enabled.
Attachment #8888651 - Attachment is obsolete: true
Attachment #8889274 - Flags: review?(sphink)
(In reply to Paul Bone [:pbone] from comment #6)
> The limit is 4GB, it's measured in KB so it looks arbitrary.  I'll use
> CheckedInt though, I think that will be clearer, at least it'll be clear
> that I've chosen the limits of the integer type.  (I copied a pattern in
> code nearby, so I'll revise it also.)

Oh, sorry, I stupidly read as 400000 *decimal*, which makes no sense. But yeah, CheckedInt is probably easier to follow (even though it is quite a bit of clutter in its own right.)

> Yes, I see the preference in about:config.  I didn't realize that there were
> yet more preferences that wern't shown there.  But I guess by omitting this
> from all.js it will now be hidden unless someone sets it.  Is there a

Oh, sorry, I wasn't proposing to omit it entirely. I think it's probably good to have it listed, so it shows up in about:config; I just wanted the default to be -1 instead of the actual value. (And yes, we have lots of sekrit preferences that are not listed in about:config; you have to manually create them with some value. But although I balk at having the actual value here, I don't feel a need to be secretive about the existence of this tuning param.)

> standard place to document the preference name?  I'll put a note about it in

I don't know, but you're right, it really should be documented somewhere. Maybe do a web search for related config names? Or a site:developer.mozilla.org search? (I wouldn't bother with searching with MDN's search box; in my experience, it's mostly useless. But Google does a good job with a site search.)

> GCSchedulingTunables which is where I'd like to concentrate all the tunables
> within the SpiderMonkey code.  We should eventually do this with all the
> others.

Sounds great to me!
Comment on attachment 8889274 [details] [diff] [review]
(part 2) - Add a pref for nursery size, r=sfink.

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

::: dom/base/nsJSEnvironment.cpp
@@ +2606,5 @@
> +SetMemoryNurseryMaxPrefChangedCallback(const char* aPrefName, void* aClosure)
> +{
> +  int32_t pref = Preferences::GetInt(aPrefName, -1);
> +  // handle overflow and negative pref values
> +  CheckedInt<uint32_t> max = CheckedInt<uint32_t>(pref) * 1024;

It bothers me a bit that we have some settings in megabytes, some in kilobytes, with no way of distinguishing them. But maybe that's a lost cause? And I wouldn't want this one to be in megabytes; seems like sub-MB granularity is desirable here. Hm... what would you think of calling this javascript.options.mem.nursery.max_kb?

r=me whatever you choose. (But if you do decide to update the name, please update it in all.js too!) And I *would* prefer to have it back in all.js.

Thanks, this looks good.
Attachment #8889274 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] [:s:] from comment #8)
> (In reply to Paul Bone [:pbone] from comment #6)
> > The limit is 4GB, it's measured in KB so it looks arbitrary.  I'll use
> > CheckedInt though, I think that will be clearer, at least it'll be clear
> > that I've chosen the limits of the integer type.  (I copied a pattern in
> > code nearby, so I'll revise it also.)
> 
> Oh, sorry, I stupidly read as 400000 *decimal*, which makes no sense. But
> yeah, CheckedInt is probably easier to follow (even though it is quite a bit
> of clutter in its own right.)

Yep, that's part of why I followed your suggestion.  It's clear exactly what the limits of CheckedInt are.

> > Yes, I see the preference in about:config.  I didn't realize that there were
> > yet more preferences that wern't shown there.  But I guess by omitting this
> > from all.js it will now be hidden unless someone sets it.  Is there a
> 
> Oh, sorry, I wasn't proposing to omit it entirely. I think it's probably
> good to have it listed, so it shows up in about:config; I just wanted the
> default to be -1 instead of the actual value. (And yes, we have lots of
> sekrit preferences that are not listed in about:config; you have to manually
> create them with some value. But although I balk at having the actual value
> here, I don't feel a need to be secretive about the existence of this tuning
> param.)

Okay.  I can see pros and cons, but mostly it's fine.  There's already a "will void your warranty" warning for about:config and I'm sure there's worse things in there.

> > standard place to document the preference name?  I'll put a note about it in
> 
> I don't know, but you're right, it really should be documented somewhere.
> Maybe do a web search for related config names? Or a
> site:developer.mozilla.org search? (I wouldn't bother with searching with
> MDN's search box; in my experience, it's mostly useless. But Google does a
> good job with a site search.)

Okay, I didn't find any.  At least it's in all.js.
(In reply to Steve Fink [:sfink] [:s:] from comment #9)
> Comment on attachment 8889274 [details] [diff] [review]
> (part 2) - Add a pref for nursery size, r=sfink.
> 
> Review of attachment 8889274 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsJSEnvironment.cpp
> @@ +2606,5 @@
> > +SetMemoryNurseryMaxPrefChangedCallback(const char* aPrefName, void* aClosure)
> > +{
> > +  int32_t pref = Preferences::GetInt(aPrefName, -1);
> > +  // handle overflow and negative pref values
> > +  CheckedInt<uint32_t> max = CheckedInt<uint32_t>(pref) * 1024;
> 
> It bothers me a bit that we have some settings in megabytes, some in
> kilobytes, with no way of distinguishing them. But maybe that's a lost
> cause? And I wouldn't want this one to be in megabytes; seems like sub-MB
> granularity is desirable here. Hm... what would you think of calling this
> javascript.options.mem.nursery.max_kb?

Yes, I noticed the nearby settings were in megabytes, and I would have chosen that except I think having finer grained control in this case is important.  However due to the chunk size that's moot on desktop since it uses 1MB chunks, on mobile they're 256KB.  This could also change and using kilobytes makes sense in that case also.

> 
> r=me whatever you choose. (But if you do decide to update the name, please
> update it in all.js too!) And I *would* prefer to have it back in all.js.

Will do.
sfink reviewed an earlier version of this patch and said "r=me whatever you choose."
Attachment #8889274 - Attachment is obsolete: true
Attachment #8889717 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30c0fb1d0997
(part 1) - Update comments to reflect API changes and a changed, r=sfink.
https://hg.mozilla.org/integration/mozilla-inbound/rev/974fd5c79451
(part 2) - Add a pref for nursery size, r=sfink.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/30c0fb1d0997
https://hg.mozilla.org/mozilla-central/rev/974fd5c79451
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.