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

Status: RESOLVED FIXED

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

Description

[Paul Bone [:pbone]]

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.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 1) Remove Nursery::exists r=nbp

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

Comment 2

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 2) Calculate the nursery's maximum size in bytes r=jonco

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

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 3) Nursery::roundSize should be a static member r=jonco

Depends on D39281

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 4) Use Nursery::roundSize() whenever setting the nursery size r=jonco

Depends on D39282

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 5) Rename a variable r=jonco

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

Depends on D39283

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 6) Remove Nursery::chunkCountLimit() r=jonco

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

Depends on D39284

Comment 7

[Jon Coppeard (:jonco)]

(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?

Comment 8

[Paul Bone [: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?

Comment 9

[Paul Bone [: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.

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 7) Factor out a common assertion r=jonco

Depends on D39285

Comment 11

[Jon Coppeard (:jonco)]

(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?

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1568740 - (part 8) Add a test r=jonco

Depends on D39473

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/251f98554914
https://hg.mozilla.org/mozilla-central/rev/7b6e6a8c7bec
https://hg.mozilla.org/mozilla-central/rev/5c978eb8f1af
https://hg.mozilla.org/mozilla-central/rev/bb6a6819da31
https://hg.mozilla.org/mozilla-central/rev/cd9d586620b2
https://hg.mozilla.org/mozilla-central/rev/ae8f7739a47a
https://hg.mozilla.org/mozilla-central/rev/7ec72ad52389
https://hg.mozilla.org/mozilla-central/rev/d681969e4480

Comment 15

[Alexandru Michis [:malexandru]]

Paul, since these changes landed Bug 1567793 started perma-failling: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=os%2Cx%2C10.14%2Cshippable%2Copt%2Cmochitests%2Ctest-macosx1014-64-shippable%2Fopt-mochitest-devtools-webreplay-e10s%2Cm%28dt-wr%29&fromchange=99898d62af446cb6cff61223ae64e2b7e8ce4a61&tochange=a2cd6653f8569267b6e807a9e038c4174a110a07&selectedJob=259890155

Could you please take a look at it?

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ab4193c798
Backed out changesets 7ec72ad52389 to 251f98554914 for causing bug 1571439

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac0a35be0d3
Backed out changeset d681969e4480 too , missed from previous commit

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d9ab4193c7986173aecc80e7a68d7430024f4bf3
https://hg.mozilla.org/mozilla-central/rev/8ac0a35be0d35f9f5aba38ed841cb3d03a2ae3c8

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/11042db2e05e
https://hg.mozilla.org/mozilla-central/rev/5d5db176ff49
https://hg.mozilla.org/mozilla-central/rev/8aa6b94125f3
https://hg.mozilla.org/mozilla-central/rev/f086a9bc71b6
https://hg.mozilla.org/mozilla-central/rev/e31e7762b04b
https://hg.mozilla.org/mozilla-central/rev/e7596cc41562
https://hg.mozilla.org/mozilla-central/rev/de7e00f6cc24
https://hg.mozilla.org/mozilla-central/rev/1d2a2360d4df