Closed Bug 1422734 Opened 2 years ago Closed 2 years ago

move --enable-small-chunk-size to moz.configure

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(2 files)

No description provided.
The setting claims that it was for B2G.  I know we've been removing B2G-related
code, but this setting actually seems pretty generic, and therefore would be
valuable to retain, rather than removing it outright.  Maybe the JS GC people
would feel differently, asking Jon for feedback.
Attachment #8934152 - Flags: review?(core-build-config-reviews)
Attachment #8934152 - Flags: feedback?(jcoppeard)
Comment on attachment 8934152 [details] [diff] [review]
move --enable-small-chunk-size to moz.configure

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

This gets set in mobile/android/confvars.sh. That needs to be reflected in mobile/android/moz.configure so it's not lost with this change.
Attachment #8934152 - Flags: review?(core-build-config-reviews)
Comment on attachment 8934152 [details] [diff] [review]
move --enable-small-chunk-size to moz.configure

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

We've talked about reducing the chunk size for other reasons (nursery per zone group) so I think this is worth retaining.
Attachment #8934152 - Flags: feedback?(jcoppeard) → feedback+
(In reply to Chris Manchester (:chmanchester) from comment #2)
> This gets set in mobile/android/confvars.sh. That needs to be reflected in
> mobile/android/moz.configure so it's not lost with this change.

Good catch.  I think I'd prefer to set it in js/moz.configure, with some sort of checking for Android when choosing the default; that feels a little more inline with how we handle other configure options.  WDYT?
Flags: needinfo?(cmanchester)
(In reply to Nathan Froyd [:froydnj] (doing reviews while in Austin) from comment #4)
> (In reply to Chris Manchester (:chmanchester) from comment #2)
> > This gets set in mobile/android/confvars.sh. That needs to be reflected in
> > mobile/android/moz.configure so it's not lost with this change.
> 
> Good catch.  I think I'd prefer to set it in js/moz.configure, with some
> sort of checking for Android when choosing the default; that feels a little
> more inline with how we handle other configure options.  WDYT?

I would leave this patch as is, but simply add `imply_option('--enable-small-chunk-size', True)` to `android/moz.configure`. This is how we've done confvars.sh things so far, it preserves the behavior, because things set in confvars.sh override any attempts to set the option. 

Really though, allowing people to set/unset this in mozconfigs probably wouldn't do any real harm, I'll leave it up to you.
Flags: needinfo?(cmanchester)
As suggested.
Attachment #8936899 - Flags: review?(cmanchester)
Attachment #8936899 - Flags: review?(cmanchester) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b21a98026b7
move --enable-small-chunk-size to moz.configure; r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/3b21a98026b7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.