Closed Bug 1568740 Opened 2 years ago Closed 2 years ago

When forcing the nursery to shrink with gcparam() it doesn't shrink fast enough

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: pbone, Assigned: pbone)

References

Details

Attachments

(8 files)

Maximum nursery size is sometimes counted in chunks rather than bytes. This can lead to a case where the caller expects the nursery to shrink because they changed the
maxNurseryBytes parameter to shrink it, but the nursery shrinks to only 512KB when the maximum is smaller.

Nursery::exists() behaved exactly the same as Nursery::isEnabled() so use
the latter.

We used to calculate the nursery's maximum size in chunks, and track the
maximum number of chunks permitted. This could lead to some unexpected
behaviour with a larger nursery than requested. Calculating it in bytes is
simpler, avoids this problem, and is more in-line with other calculations.

Depends on D39280

The new variable, 'newMinNurseryBytes' is more in-line with the other
'newMaxNurseryBytes'.

Depends on D39283

This method & the chunkCountLimit_ variable are unnecessary. The maximum
permitted nursery size is tracked in the GCSchedulingTunables class.

Depends on D39284

(In reply to Paul Bone [:pbone] from comment #0)
Not directly related to any of the patches: can you add a test case for this?

Flags: needinfo?(pbone)

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

(In reply to Paul Bone [:pbone] from comment #0)
Not directly related to any of the patches: can you add a test case for this?

It's not convenient. we'd need to add a new runtime parameter to query the current nursery size. I don't think its worth while to add the parameter just for a test. but maybe we want the parameter anyway? What do you think?

Flags: needinfo?(pbone)

(In reply to Paul Bone [:pbone] from comment #8)

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

(In reply to Paul Bone [:pbone] from comment #0)
Not directly related to any of the patches: can you add a test case for this?

It's not convenient. we'd need to add a new runtime parameter to query the current nursery size. I don't think its worth while to add the parameter just for a test. but maybe we want the parameter anyway? What do you think?

If we did add a new parameter we could use it to write a test case for Bug 1388701 also.

(In reply to Paul Bone [:pbone] from comment #8)

It's not convenient. we'd need to add a new runtime parameter to query the current nursery size.

We have the JSGC_BYTES parameter for heap size, so having one for nursery size too makes sense I think.

Can you add this and add tests that the nursery resizes correctly when the size parameters are changed?

Depends on: 1569151
Depends on: 1569840

Depends on D39473

Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/251f98554914
(part 1) Remove Nursery::exists r=nbp
https://hg.mozilla.org/integration/autoland/rev/7b6e6a8c7bec
(part 2) Calculate the nursery's maximum size in bytes r=jonco
https://hg.mozilla.org/integration/autoland/rev/5c978eb8f1af
(part 3) Nursery::roundSize should be a static member r=jonco
https://hg.mozilla.org/integration/autoland/rev/bb6a6819da31
(part 4) Use Nursery::roundSize() whenever setting the nursery size r=jonco
https://hg.mozilla.org/integration/autoland/rev/cd9d586620b2
(part 5) Rename a variable r=jonco
https://hg.mozilla.org/integration/autoland/rev/ae8f7739a47a
(part 6) Remove Nursery::chunkCountLimit() r=jonco
https://hg.mozilla.org/integration/autoland/rev/7ec72ad52389
(part 7) Factor out a common assertion r=jonco
https://hg.mozilla.org/integration/autoland/rev/d681969e4480
(part 8) Add a test r=jonco
Regressions: 1567793
Backout by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ab4193c798
Backed out changesets 7ec72ad52389 to 251f98554914 for causing bug 1571439
Backout by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac0a35be0d3
Backed out changeset d681969e4480 too , missed from previous commit
Depends on: 1567793
No longer regressions: 1567793
Pushed by pbone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11042db2e05e
(part 1) Remove Nursery::exists r=nbp
https://hg.mozilla.org/integration/autoland/rev/5d5db176ff49
(part 2) Calculate the nursery's maximum size in bytes r=jonco
https://hg.mozilla.org/integration/autoland/rev/8aa6b94125f3
(part 3) Nursery::roundSize should be a static member r=jonco
https://hg.mozilla.org/integration/autoland/rev/f086a9bc71b6
(part 4) Use Nursery::roundSize() whenever setting the nursery size r=jonco
https://hg.mozilla.org/integration/autoland/rev/e31e7762b04b
(part 5) Rename a variable r=jonco
https://hg.mozilla.org/integration/autoland/rev/e7596cc41562
(part 6) Remove Nursery::chunkCountLimit() r=jonco
https://hg.mozilla.org/integration/autoland/rev/de7e00f6cc24
(part 7) Factor out a common assertion r=jonco
https://hg.mozilla.org/integration/autoland/rev/1d2a2360d4df
(part 8) Add a test r=jonco
No longer depends on: 1567793
Flags: needinfo?(pbone)
Regressions: 1584181
You need to log in before you can comment on or make changes to this bug.