1384010 - Expose zoneAllocThresholdFactor() in about:config

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Paul Bone [:pbone]]

Expose zoneAllocThresholdFactor() and tunables.zoneAllocThresholdFactorAvoidInterrupt() in about:config

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1384010 - Create prefs for allocation threshold factors r=jonco

Comment 2

[Jon Coppeard (:jonco)]

Comment on attachment 8894766 [details] [diff] [review]
Bug 1384010 - Create prefs for allocation threshold factors r=jonco

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

Looks good.

BTW, you should only put "r=jonco" on the patch *after* I've reviewed it ;)

::: js/src/jsgc.cpp
@@ +1295,5 @@
> +      case JSGC_ALLOCATION_THRESHOLD_FACTOR: {
> +        float newFactor = value / 100.0;
> +        if (newFactor <= 0.1 || newFactor > 1.0)
> +            return false;
> +        zoneAllocThresholdFactor_ = newFactor;

Why are we using float for these now?  I think I missed the discussion about this.

Comment 3

[Paul Bone [:pbone]]

(In reply to Jon Coppeard (:jonco) from comment #2)
> Comment on attachment 8894766 [details] [diff] [review]
> Bug 1384010 - Create prefs for allocation threshold factors r=jonco
> 
> Review of attachment 8894766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> BTW, you should only put "r=jonco" on the patch *after* I've reviewed it ;)

So + means you've reviewed it and like it, and = also means you're reviewed it and liked it?

It's not 100% clear between these, and what to put in the patch and what to put on bugzilla and how it ends up landing.  Maybe the new thing will clear some of that up when it's ready.  Should I have put r?jonco or just left this off the patch headers?

> ::: js/src/jsgc.cpp
> @@ +1295,5 @@
> > +      case JSGC_ALLOCATION_THRESHOLD_FACTOR: {
> > +        float newFactor = value / 100.0;
> > +        if (newFactor <= 0.1 || newFactor > 1.0)
> > +            return false;
> > +        zoneAllocThresholdFactor_ = newFactor;
> 
> Why are we using float for these now?  I think I missed the discussion about
> this.

Sorry you were offline from IRC, I asked sfink why we use double everywhere when that's way more precision than we need. (for numbers like 0.9 and 3.0).  He said it comes from a historical thing when double may have been faster than float but nobody was really sure if that was ever true, when it existed, or why C typedefs weren't use to choose the architectures fastest option.  I changed them to float because I didn't see why they needed to be double.  sfink approved in bug 1386660.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

(In reply to Paul Bone [:pbone] from comment #3)
> > BTW, you should only put "r=jonco" on the patch *after* I've reviewed it ;)
> 
> So + means you've reviewed it and like it, and = also means you're reviewed
> it and liked it?
> 
> It's not 100% clear between these, and what to put in the patch and what to
> put on bugzilla and how it ends up landing.  Maybe the new thing will clear
> some of that up when it's ready.  Should I have put r?jonco or just left
> this off the patch headers?

r=reviewer is what goes into the commit log as an easy reference, and also for things like commit hooks (e.g. to ensure that a DOM peer has looked at a patch that touches the DOM). Putting it in the patch header before a patch has actually been reviewed increases the risk of it landing without review.

People sometimes put r=UNREVIEWED or some variant in the header while review is pending. You'll also sometimes see people comment (on Bugzilla or on IRC) with r=me for simple changes (where `me` is the reviewer, not the patch author).

If a patch header doesn't contain the reviewer and you set checkin-needed, sheriffs will add the reviewer before landing the changes. But this is a manual process, so if you get review comments that need to be addressed you should either leave it out altogether, reupload each part with the header adjusted (it's possible to edit patches on Bugzilla itself, but unfortunately this will quote the whole patch in a comment), or land the patches yourself.

r+, r? and r- are the Bugzilla review states, but those should never go in the patch header. Oh, and if you get a comment like "r=me with the following", you can set r+ on the updated patch yourself with a comment like "Carrying forward r=reviewer" (in that case you should probably add r=reviewer to the patch header).

I think that's it. Oh, one thing I ran into once: if you use checkin-needed, make sure the Try run in the bug is up-to-date, as sheriffs will sometimes use it to land your patches.

Comment 5

[Paul Bone [:pbone]]

(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #4)

Thanks for all the info, but especially this.

> I think that's it. Oh, one thing I ran into once: if you use checkin-needed,
> make sure the Try run in the bug is up-to-date, as sheriffs will sometimes
> use it to land your patches.

I would have had no idea and that could have been dangerous.  Thanks for letting me know.

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1384010 - Create prefs for allocation threshold factors r=jonco

This patch was already reviewed by jonco, this re-upload was after rebasing it on top of Bug 1386660

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1384010 - Create prefs for allocation threshold factors r=jonco

Updated to apply cleanly after Bug 1386660 landed.  r+ carried forward.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/304dc7a9b264
Create prefs for allocation threshold factors. r=jonco

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/304dc7a9b264