1792722 - MaxHeapGrowth limit calculation for balanced heap limits is wrong

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Jon Coppeard (:jonco)]

pavpanchekha pointed out that I messed up the calculation in bug 1790336 and it doesn't do what I intended. I think the final W should be inside the bracket not outside:

https://searchfox.org/mozilla-central/source/js/src/gc/Scheduling.cpp#662

I'll need to re-test to see if this actually fixes the benchmark issues or back out the original bug and look for another solution.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1790336

Comment 2

[Pascal Chevrel:pascalc (PTO until April 26)]

Jon, is that something that will require an uplift to 106? Thanks

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Pascal Chevrel:pascalc from comment #2)
No, balanced heap limits are not enabled on 106 (and will shortly be disabled on 107 too).

Comment 4

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Jon Coppeard (:jonco) from comment #3)

(In reply to Pascal Chevrel:pascalc from comment #2)
No, balanced heap limits are not enabled on 106 (and will shortly be disabled on 107 too).

Thanks, setting 106 status to disabled then.

Comment 5

[Jon Coppeard (:jonco)]

Fixing this regresses benchmark results.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1790336

Comment 7

[Donal Meehan [:dmeehan]]

(In reply to Jon Coppeard (:jonco) from comment #3)

(In reply to Pascal Chevrel:pascalc from comment #2)
No, balanced heap limits are not enabled on 106 (and will shortly be disabled on 107 too).

:jonco, trying to figure out the impact for 107, if you could help me out?
I found the following in beta

pref("javascript.options.mem.gc_heap_growth_factor", 50);

The value differs from release (i.e. 50 in beta, and 27 in release)

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Donal Meehan [:dmeehan] from comment #7)
The gc_heap_growth_factor parameter is not used unless javascript.options.mem.gc_balanced_heap_limits is set to true. There's no impact from this value changing.

Comment 9

[Donal Meehan [:dmeehan]]

(In reply to Jon Coppeard (:jonco) from comment #8)

(In reply to Donal Meehan [:dmeehan] from comment #7)
The gc_heap_growth_factor parameter is not used unless javascript.options.mem.gc_balanced_heap_limits is set to true. There's no impact from this value changing.

Thanks. I didn't spot pref.
javascript.options.mem.gc_balanced_heap_limits is set to false in beta so marking 107 as disabled.

Comment 10

[Jim Mathies [:jimm]]

Do we have a meta for gc_heap_growth_factor we can hook this up as a dependency on?

Comment 11

[Jon Coppeard (:jonco)]

Attachment: Bug 1792722 - Fix balanced heap limit calculation r?sfink

This should limit heap size to some multiple of the previous size. |f| is a
size in bytes and should not be multiplied by W.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c0c8c094e5b
Fix balanced heap limit calculation r=sfink

Comment 13

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/5c0c8c094e5b

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.

Comment 15

[Jon Coppeard (:jonco)]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #14)
Balanced heap limits are not enabled by default so nothing needs to be uplifted here.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jonco, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox109 to wontfix.

For more information, please visit auto_nag documentation.