1386660 - Refactor and fix the code that sets the GC prefs

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Paul Bone [:pbone]]

Refactor and generally improve the rather ad-hoc pref setting code.

Also fix code that either deliberately or in some cases accidentally resets a preference by setting it to -1 and hoping that SpiderMonkey does the right thing.

Comment 1

[Jon Coppeard (:jonco)]

E.g. https://searchfox.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#2602

Comment 2

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 1) - Update a comment to reflect current code r=jonco

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 2) - Use float for zoneAllocThresholdFactor r=sfink

I've asked Steve to review this since we talked about it on IRC a few days ago.

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 3) - Provide code to reset SM/GC parameters r=jonco

I'm having problems with this patch and clang with address sanitisation.  (Thanks try!). So maybe wait until I figure it out before reviewing it?

https://treeherder.mozilla.org/logviewer.html#?job_id=120400145&repo=try&lineNumber=2200

But if someone knows what I've done wrong (looks like it's a C++ thing that I don't know yet) I'd appreciate it.

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 4) - Refactor pref code in nsJSEnvironment.cpp r=jonco

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8893226 [details] [diff] [review]
Bug 1386660 (part 1) - Update a comment to reflect current code r=jonco

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

::: js/src/gc/GCRuntime.h
@@ +155,4 @@
>      ActiveThreadData<size_t> gcMaxNurseryBytes_;
>  
>      /*
> +     * The base value used to compute zone->trigger.gcTriggerBytes(). When

We should also change this to say zone->threshold as that was wrong too :)

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8893229 [details] [diff] [review]
Bug 1386660 (part 4) - Refactor pref code in nsJSEnvironment.cpp r=jonco

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

Thanks for cleaning this up.  This looks good to me, but you'll need a review from DOM peer for this.

::: dom/base/nsJSEnvironment.cpp
@@ +2622,5 @@
> +    ResetGCParameter((JSGCParamKey)(uintptr_t)aClosure);
> +}
> +
> +static void
> +SetMemoryPrefChangedCallbackNat(const char* aPrefName, void* aClosure)

What does 'Nat' mean here?  I think 'Int' would be clearer.

@@ +2796,5 @@
> +                                       "javascript.options.mem.max",
> +                                       (void*)JSGC_MAX_BYTES);
> +  Preferences::RegisterCallbackAndCall(SetMemoryPrefChangedCallbackKB,
> +                                       "javascript.options.mem.nursery.max_kb",
> +                                       (void*)JSGC_MAX_NURSERY_BYTES);

Pre-existing question: I wonder why this is in KB when we only allocate the nursery in chunks of 1 MiB?

Comment 8

[Olli Pettay [:smaug]]

Comment on attachment 8893229 [details] [diff] [review]
Bug 1386660 (part 4) - Refactor pref code in nsJSEnvironment.cpp r=jonco

Please don't use *Nat (I have no idea what it means). *Int is clear. 

Always {} with if.
So, 
if (...) {
  ...
} else {
  ...
}

Local variable naming should use camelCase. So, not pref_mb, but prefMB or so.

Since SetMemoryPrefChangedCallbackKB is used only by one pref, could you not change the name but just keep the name SetMemoryNurseryMaxPrefChangedCallback (the change to the behavior looks ok).
The behavior of SetMemoryPrefChangedCallbackKB is hard to understand from its name.

Comment 9

[Paul Bone [:pbone]]

> > +
> > +static void
> > +SetMemoryPrefChangedCallbackNat(const char* aPrefName, void* aClosure)
> 
> What does 'Nat' mean here?  I think 'Int' would be clearer.

Natural number, but that's actually incorrect, Int is better.

> @@ +2796,5 @@
> > +                                       "javascript.options.mem.max",
> > +                                       (void*)JSGC_MAX_BYTES);
> > +  Preferences::RegisterCallbackAndCall(SetMemoryPrefChangedCallbackKB,
> > +                                       "javascript.options.mem.nursery.max_kb",
> > +                                       (void*)JSGC_MAX_NURSERY_BYTES);
> 
> Pre-existing question: I wonder why this is in KB when we only allocate the
> nursery in chunks of 1 MiB?

Because sometimes it is allocated in chunks of 256K, like on mobile.

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 1) - Update a comment to reflect current code r=jonco

Updated based on jonco's review.

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 3) - Provide code to reset SM/GC parameters r=jonco

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 4) - Refactor pref code in nsJSEnvironment.cpp r=jonco

Updated based on review feedback

Comment 13

[Paul Bone [:pbone]]

Hi jonco / sfink.

In some cases default values are specified both in SpiderMonkey (in GCRuntime.h) and in Firefox (all.js), often they're the same value, for example javascript.options.mem.gc_allocation_threshold_mb.  When they're identical should I set the Firefox ones to -1 so that we're not repeating this in multiple places?  I'm happy to add another patch for that change.

Thanks.

Comment 14

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #9)
> Because sometimes it is allocated in chunks of 256K, like on mobile.

That's a good point.

Comment 15

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #4)
> I'm having problems with this patch and clang with address sanitisation. 

I think this is to do with having the defaults as static constexpr members that are initialised in the body of the class.  Before  constexpr, you would have used static const and inline initialisation was only allowed for integral values.  It looks like what you have here should work, but the linker error indicates that somehow we're trying to create a reference to this member and that  requires it to be defined outside the class too.

I don't see why this is happening but it's probably some complexity of the ActiveThreadOrGCTaskData<> template that I don't understand.

The simplest thing would be to move these constants out of the class definition (and you can just use const rather than constexpr in that case too).

Comment 16

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #13)
> When they're
> identical should I set the Firefox ones to -1 so that we're not repeating
> this in multiple places? 

What do you think Steve?  I know it's duplication but I think I'd rather have all the browser defaults in one place in the browser.

Comment 17

[Jon Coppeard (:jonco)]

Comment on attachment 8894750 [details] [diff] [review]
Bug 1386660 (part 3) - Provide code to reset SM/GC parameters r=jonco

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

::: js/src/gc/GCRuntime.h
@@ +235,5 @@
>    public:
> +    /*
> +     * The casts here are necessary to prevent C++ from generating references
> +     * to these types and confusing the linker in asan builds.
> +     */

Ah, I see I should have read this patch first before commenting above.

Please move the constants out of the class body.  That should fix things without casts.

::: js/src/jsgc.cpp
@@ +1327,5 @@
> +        defaultTimeBudget_ =
> +            static_cast<int64_t>(SliceBudget::UnlimitedTimeBudget);
> +        break;
> +      case JSGC_MARK_STACK_LIMIT:
> +        setMarkStackLimit(0xffffffff, lock);

The initial value is actually set to size_t(-1) so we should set that here.

@@ +1367,5 @@
> +        highFrequencyLowLimitBytes_ =
> +            static_cast<uint64_t>(highFrequencyLowLimitBytesDefault);
> +        if (highFrequencyLowLimitBytes_ >= highFrequencyHighLimitBytes_)
> +            highFrequencyHighLimitBytes_ = highFrequencyLowLimitBytes_ + 1;
> +        MOZ_ASSERT(highFrequencyHighLimitBytes_ > highFrequencyLowLimitBytes_);

This duplicates a lot of the verification logic from setParameter.

Where necessary, could you split out separate methods to set/verify the individual parameters and call them from both setParameter and resetParameter?

Comment 18

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #15)
> (In reply to Paul Bone [:pbone] from comment #4)
> > I'm having problems with this patch and clang with address sanitisation. 
> 
> I think this is to do with having the defaults as static constexpr members
> that are initialised in the body of the class.  Before  constexpr, you would
> have used static const and inline initialisation was only allowed for
> integral values.  It looks like what you have here should work, but the
> linker error indicates that somehow we're trying to create a reference to
> this member and that  requires it to be defined outside the class too.

I experimented and got the same problem with const and constexpr (at least for integers).  So I made the integers const and the floats/doubles constexpr.

> I don't see why this is happening but it's probably some complexity of the
> ActiveThreadOrGCTaskData<> template that I don't understand.

I found that either assigning to one of these, or calling its constructor, would attempt to use const int& rather than just int, and therefore create a reference to it.  At least for clang with address sanitisation.  GCC worked fine, I didn't try clang without address sanitisation.  Hence the patch with the casts works.

> The simplest thing would be to move these constants out of the class
> definition (and you can just use const rather than constexpr in that case
> too).

Okay, I'll fix it in the morning.

Comment 19

[Steve Fink [:sfink] [:s:]]

(In reply to Jon Coppeard (:jonco) from comment #16)
> (In reply to Paul Bone [:pbone] from comment #13)
> > When they're
> > identical should I set the Firefox ones to -1 so that we're not repeating
> > this in multiple places? 
> 
> What do you think Steve?  I know it's duplication but I think I'd rather
> have all the browser defaults in one place in the browser.

Hm, maybe I should change my opinion on this.

I definitely want the settings to appear in about:config, which I think means listing them in all.js.

If they are always going to be identical between the shell and the browser, then it seems like repeating the logic to result in the same settings in both places is error-prone, so we ought to use -1 or whatever in all.js to use the default.

But if we *do* imagine that we might want browser-specific settings, ones that won't always match the compiled-in constants, then it makes sense to repeat the logic in both places. ("Logic" as in, "if on mobile set to X else set to Y".)

So for me, it hinges on whether browser-specific settings make sense. And when I think through an example, I unfortunately hit some complexity: we are seeing some guaranteed nursery resizing that slows down browser startup, so it seems like our initial nursery size should be larger in the browser. Yet workers probably ought to start small. So yes, it seems like browser-specific settings make sense, but to do it right might require more complexity than we currently have. (Or do we have worker-specific or startup-specific settings? I don't actually know offhand.)

I guess that means I'm voting for putting the actual values in all.js, and filing bugs for splitting out settings for different configurations if we don't already.

Comment 20

[Paul Bone [:pbone]]

Why not put all the defaults that are identical in GCRuntime.h, using -1 in all.js, and when they're different giving the different value in all.js and a comment about why it's different.  If we can decide I can write a patch and add it to this bug.  Maybe jonco and I will talk about it tonight in our meeting.

Startup / worker settings is a separate issue IMHO, and should probably be done case-by-case.  For example SpiderMonkey could have two different default nursery sizes, one for the main thread and one for workers.  Then Firefox should mostly defer to SpiderMonkey except perhaps during startup.  I'd like to discuss this in Bug 1387950, since I think it is a separate issue, and I don't what it's discussion to get lost after this bug is closed.

Comment 21

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 3) - Provide code to reset SM/GC parameters

Comment 22

[Jon Coppeard (:jonco)]

Comment on attachment 8895320 [details] [diff] [review]
Bug 1386660 (part 3) - Provide code to reset SM/GC parameters

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

Great, thanks for the updates.

::: js/src/jsgc.cpp
@@ +1302,4 @@
>      return true;
>  }
>  
> +void GCSchedulingTunables::setHighFrequencyLowLimit(uint64_t newLimit) {

nit: When defining a method outside the class statement the type goes on a separate line here as does the open brace, e.g. like this:

void
GCSchedulingTunables::setHighFrequencyLowLimit(uint64_t newLimit) 
{

@@ +1329,5 @@
> +        minEmptyChunkCount_ = maxEmptyChunkCount_;
> +    MOZ_ASSERT(maxEmptyChunkCount_ >= minEmptyChunkCount_);
> +}
> +
> +static const size_t gcZoneAllocThresholdBaseDefault = 30 * 1024 * 1024;

I can't find where it says it in the coding style guide but in general constants start with a capital letter e.g GCZoneAllocThresholdDefault.

Comment 23

[Olli Pettay [:smaug]]

proper values in all.js are self-documenting for whoever uses about:config to tweak them.

Comment 24

[Paul Bone [:pbone]]

(In reply to Olli Pettay [:smaug] from comment #23)
> proper values in all.js are self-documenting for whoever uses about:config
> to tweak them.

Jon and I met last night and taking into account the other comments on this thread we have decided to duplicate these values, so that they can be seen in about:config.  With your vote added it's pretty clear that this is preferred.

Comment 25

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #22)
> Comment on attachment 8895320 [details] [diff] [review]
> Bug 1386660 (part 3) - Provide code to reset SM/GC parameters
> 
> Review of attachment 8895320 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great, thanks for the updates.
> 
> ::: js/src/jsgc.cpp
> @@ +1302,4 @@
> >      return true;
> >  }
> >  
> > +void GCSchedulingTunables::setHighFrequencyLowLimit(uint64_t newLimit) {
> 
> nit: When defining a method outside the class statement the type goes on a
> separate line here as does the open brace, e.g. like this:
> 
> void
> GCSchedulingTunables::setHighFrequencyLowLimit(uint64_t newLimit) 
> {
> 
> @@ +1329,5 @@
> > +        minEmptyChunkCount_ = maxEmptyChunkCount_;
> > +    MOZ_ASSERT(maxEmptyChunkCount_ >= minEmptyChunkCount_);
> > +}
> > +
> > +static const size_t gcZoneAllocThresholdBaseDefault = 30 * 1024 * 1024;
> 
> I can't find where it says it in the coding style guide but in general
> constants start with a capital letter e.g GCZoneAllocThresholdDefault.

Sorry, I should have at least noticed both of these conventions, even if I didn't remember what I read on the style document. I was careless.

Comment 26

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 3) - Provide code to reset SM/GC parameters r=jonco

jonco reviewed this patch already, I've made the minor changes he requested in that review.

Comment 27

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 5) - Add constants to avoid multiple hard-coded values

Comment 28

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 6) - Clarify relationships between prefs, JSGC params, and their defaults

Comment 29

[Paul Bone [:pbone]]

The try job for these changes is https://treeherder.mozilla.org/#/jobs?repo=try&revision=91646bc62f788ec0c62acfdd82072e66991752a2

Comment 30

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 6) - Clarify relationships between prefs, JSGC params, and their defaults

Comment 31

[Paul Bone [:pbone]]

After fixing a bug the updated try job is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b267beb55909aeb836f913f310b61b1dac38dd81

Comment 32

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 7) - Fix build error on Windows

Comment 33

[Paul Bone [:pbone]]

Comment on attachment 8897301 [details] [diff] [review]
Bug 1386660 (part 7) - Fix build error on Windows

After fixing this I ran try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f53353f047daaf304684fbf73dc5272a310980d4

Comment 34

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8896300 [details] [diff] [review]
Bug 1386660 (part 5) - Add constants to avoid multiple hard-coded values

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

::: js/src/jsgc.cpp
@@ +284,5 @@
> + * JSGC_SLICE_TIME_BUDGET.
> + * javascript.options.mem.gc_incremental_slice_ms
> + */
> +static const int64_t DefaultTimeBudgetDefault =
> +    SliceBudget::UnlimitedTimeBudget;

The Default...Default name bothers me for reasons that I couldn't initially put a finger on. But I think I know now.

Those two "Defaults" don't mean the same thing. It's the default setting for defaultTimeBudget_. So the first default (from defaultTimeBudget_) is "what to use for GCs where no other time budget is specified." The trailing default is "what to set the above default to if configuration does not say otherwise."

But I can't come up with a better name for either, given that they both really are defaults.

What would you think about putting all of this stuff into a namespace or struct with "default" in its name, perhaps js::gc::DefaultSettings? Then this would be DefaultSettings::DefaultTimeBudget, alongside things like DefaultSettings::MaxEmptyChunkCount etc. To me at least, that seems more understandable. (I'm not settled on the exact name or anything. JS::GCTuningDefaults is a totally different name that would work too.)

Anyway, I trust your instincts on naming, so even if you want to leave it exactly as-is, I'm fine with it.

Comment 35

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8896814 [details] [diff] [review]
Bug 1386660 (part 6) - Clarify relationships between prefs, JSGC params, and their defaults

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

Very nice! This stuff is way easier to follow. So far, I've just been searching through code whenever someone asks me about one of these.

::: js/src/gc/GCRuntime.h
@@ +1328,5 @@
>      /*
>       * Whether compacting GC can is enabled globally.
> +     *
> +     * JSGC_COMPACTING_ENABLED
> +     * pref: javascript.options.mem.gc_compacting

all of these javascript.options.mem.gc_* names make me wonder if we could rename them all to javascript.options.gc.*. The 'mem' part doesn't seem helpful. But don't hold up this bug for that.

::: js/src/jsapi.h
@@ +1855,5 @@
> +    /**
> +     * Maximum size the GC mark stack can grow to.
> +     *
> +     * Pref: none
> +     * Shell default: size_t(-1)

Can you comment what -1 means?

::: js/src/jsgc.cpp
@@ +315,4 @@
>  static const int64_t DefaultTimeBudgetDefault =
>      SliceBudget::UnlimitedTimeBudget;
>  
> +// JSGC_MODE */

Unmatched comment markers

Comment 36

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] (PTO Aug 16-23) from comment #34)
> Comment on attachment 8896300 [details] [diff] [review]
> Bug 1386660 (part 5) - Add constants to avoid multiple hard-coded values
> 
> Review of attachment 8896300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsgc.cpp
> @@ +284,5 @@
> > + * JSGC_SLICE_TIME_BUDGET.
> > + * javascript.options.mem.gc_incremental_slice_ms
> > + */
> > +static const int64_t DefaultTimeBudgetDefault =
> > +    SliceBudget::UnlimitedTimeBudget;
> 
> The Default...Default name bothers me for reasons that I couldn't initially
> put a finger on. But I think I know now.
> 
> Those two "Defaults" don't mean the same thing. It's the default setting for
> defaultTimeBudget_. So the first default (from defaultTimeBudget_) is "what
> to use for GCs where no other time budget is specified." The trailing
> default is "what to set the above default to if configuration does not say
> otherwise."
> 
> But I can't come up with a better name for either, given that they both
> really are defaults.

I agree, and me neither :-\


> What would you think about putting all of this stuff into a namespace or
> struct with "default" in its name, perhaps js::gc::DefaultSettings? Then
> this would be DefaultSettings::DefaultTimeBudget, alongside things like
> DefaultSettings::MaxEmptyChunkCount etc. To me at least, that seems more
> understandable. (I'm not settled on the exact name or anything.
> JS::GCTuningDefaults is a totally different name that would work too.)

I had this idea also, but I didn't know if this was a "normal" thing to do (C++ style wise) and didn't want to overcomplicate things.  I like js::gc::TuningDefaults (or similar).

> Anyway, I trust your instincts on naming, so even if you want to leave it
> exactly as-is, I'm fine with it.

I'll consider this, I might add another patch or just let it land the way it is.

Cheers.

Comment 37

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] (PTO Aug 16-23) from comment #35)
> Comment on attachment 8896814 [details] [diff] [review]
> Bug 1386660 (part 6) - Clarify relationships between prefs, JSGC params, and
> their defaults
> 
> Review of attachment 8896814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Very nice! This stuff is way easier to follow. So far, I've just been
> searching through code whenever someone asks me about one of these.

Cool. They've all got the JSGC_ symbol name near them so if you use editor tags or searchfox, you can follow it for more information.  This was part of my intention with cleaning it up.

> ::: js/src/gc/GCRuntime.h
> @@ +1328,5 @@
> >      /*
> >       * Whether compacting GC can is enabled globally.
> > +     *
> > +     * JSGC_COMPACTING_ENABLED
> > +     * pref: javascript.options.mem.gc_compacting
> 
> all of these javascript.options.mem.gc_* names make me wonder if we could
> rename them all to javascript.options.gc.*. The 'mem' part doesn't seem
> helpful. But don't hold up this bug for that.

I like that, but I also heard that we try to avoid renaming these.  Maybe because it breaks something else, or maybe because it breaks user's configurations.  Maybe make this a separate bug?  What are your thoughts Jon?

> ::: js/src/jsapi.h
> @@ +1855,5 @@
> > +    /**
> > +     * Maximum size the GC mark stack can grow to.
> > +     *
> > +     * Pref: none
> > +     * Shell default: size_t(-1)
> 
> Can you comment what -1 means?

Nope.  I don't know, it was like that when I got here.  :-)  I think it just means "unlimited" but in a really bad way.

Unless jonco knows what was intended I'll add a comment saying that we don't know.

Comment 38

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 5) - Add constants to avoid multiple hard-coded values r=sfink

Updated to address review comments.

I did choose to add a namespace for the defaults.

Thanks.

Comment 39

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 6) - Clarify relationships between prefs r=sfink

Updated to address review comments.

Comment 40

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 7) - Fix build error on Windows r=jonco

Updated to merge cleanly after other changes.

Comment 41

[Paul Bone [:pbone]]

(In reply to Paul Bone [:pbone] from comment #37)
> (In reply to Steve Fink [:sfink] [:s:] (PTO Aug 16-23) from comment #35)
> > Comment on attachment 8896814 [details] [diff] [review]
> > Bug 1386660 (part 6) - Clarify relationships between prefs, JSGC params, and
> > their defaults
> > 
> > Review of attachment 8896814 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Very nice! This stuff is way easier to follow. So far, I've just been
> > searching through code whenever someone asks me about one of these.
> 
> Cool. They've all got the JSGC_ symbol name near them so if you use editor
> tags or searchfox, you can follow it for more information.  This was part of
> my intention with cleaning it up.
> 
> > ::: js/src/gc/GCRuntime.h
> > @@ +1328,5 @@
> > >      /*
> > >       * Whether compacting GC can is enabled globally.
> > > +     *
> > > +     * JSGC_COMPACTING_ENABLED
> > > +     * pref: javascript.options.mem.gc_compacting
> > 
> > all of these javascript.options.mem.gc_* names make me wonder if we could
> > rename them all to javascript.options.gc.*. The 'mem' part doesn't seem
> > helpful. But don't hold up this bug for that.
> 
> I like that, but I also heard that we try to avoid renaming these.  Maybe
> because it breaks something else, or maybe because it breaks user's
> configurations.  Maybe make this a separate bug?  What are your thoughts Jon?
> 
> > ::: js/src/jsapi.h
> > @@ +1855,5 @@
> > > +    /**
> > > +     * Maximum size the GC mark stack can grow to.
> > > +     *
> > > +     * Pref: none
> > > +     * Shell default: size_t(-1)
> > 
> > Can you comment what -1 means?
> 
> Nope.  I don't know, it was like that when I got here.  :-)  I think it just
> means "unlimited" but in a really bad way.
> 
> Unless jonco knows what was intended I'll add a comment saying that we don't
> know.

Nevermind, I referred to the constant in Marker.cpp where this is defined.  I should have done that like I had done with the other defaults.  I still don't know why it's size_t(-1), but now it points at the original code for that.

Comment 42

[Paul Bone [:pbone]]

This is ready for checkin.  There are two try links in previous comments, the failing tests from the first one were repeated in the second one after fixing a problem.  Thank you.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

Can you please fold the Windows bustage fix into whatever patch caused it so bisection isn't broken?

Comment 44

[Jon Coppeard (:jonco)]

(In reply to Paul Bone [:pbone] from comment #37)
> > Can you comment what -1 means?
> 
> I think it just means "unlimited" but in a really bad way.

That is correct.  We should use SIZE_MAX instead.

Comment 45

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 2) - Use float for zoneAllocThresholdFactor r=sfink

Re-upload to fix compilation on Windows.

Comment 46

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 3) - Provide code to reset SM/GC parameters r=jonco

Re-upload to fix compilation on windows.

Comment 47

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 5) - Add constants to avoid multiple hard-coded values r=sfink

Re-upload to fix build on windows, also use SIZE_MAX rather than size_t(-1).

Comment 48

[Paul Bone [:pbone]]

Hi Ryan,

I've re-uploaded 3 of the patches since this code was moved in some of them and even if I changed 0.9 to 0.9f in the first one, it wouldn't have been clear where that code is moved to by the others.  This is why I didn't do it earlier, I wasn't sure if we considered it important to be able to bisect history (I like to be able to, personally, but I don't know what we do as an organization).  Thanks.

Comment 49

[Carsten Book [:Tomcat]]

seems there are some problems like patching file js/src/jsgc.cpp
Hunk #1 FAILED at 266
1 out of 1 hunks FAILED -- saving rejects to file js/src/jsgc.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh pref-6a 

when applying part 6

Comment 50

[Paul Bone [:pbone]]

Attachment: Bug 1386660 (part 6) - Clarify relationships between prefs r=sfink

Sorry Tomcat,

This should apply cleanly.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88057095fa2f
Part 1: Update a comment to reflect current code. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/32e58ff6a1ee
Part 2: Use float for zoneAllocThresholdFactor. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f05eb69802f
Part 3: Provide code to reset SM/GC parameters. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c53785fd4b
Part 4: Refactor pref code in nsJSEnvironment.cpp. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1fcbcb67951
Part 5: Add constants to avoid multiple hard-coded values. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed8427cb4342
Part 6: Clarify relationships between prefs. r=sfink

Comment 52

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/88057095fa2f
https://hg.mozilla.org/mozilla-central/rev/32e58ff6a1ee
https://hg.mozilla.org/mozilla-central/rev/6f05eb69802f
https://hg.mozilla.org/mozilla-central/rev/a1c53785fd4b
https://hg.mozilla.org/mozilla-central/rev/f1fcbcb67951
https://hg.mozilla.org/mozilla-central/rev/ed8427cb4342