Closed
Bug 1422734
Opened 3 years ago
Closed 3 years ago
move --enable-small-chunk-size to moz.configure
Categories
(Firefox Build System :: General, enhancement)
Firefox Build System
General
Tracking
(firefox59 fixed)
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
Details
Attachments
(2 files)
3.22 KB,
patch
|
jonco
:
feedback+
|
Details | Diff | Splinter Review |
4.64 KB,
patch
|
chmanchester
:
review+
|
Details | Diff | Splinter Review |
No description provided.
![]() |
Assignee | |
Comment 1•3 years ago
|
||
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 2•3 years ago
|
||
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 3•3 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•3 years ago
|
||
(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)
Comment 5•3 years ago
|
||
(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)
Updated•3 years ago
|
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
Comment 8•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b21a98026b7
Status: NEW → RESOLVED
Closed: 3 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment 9•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3b21a98026b7
Updated•3 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•