Closed Bug 1568740 Opened 5 years ago Closed 5 years ago

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

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla71

Tracking Flags:

Tracking

Status

firefox71

---

fixed

People

(Reporter: pbone, Assigned: pbone)

References

Details

Attachments

(8 files)

Bug 1568740 - (part 1) Remove Nursery::exists r=nbp 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 2) Calculate the nursery's maximum size in bytes r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 3) Nursery::roundSize should be a static member r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 4) Use Nursery::roundSize() whenever setting the nursery size r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 5) Rename a variable r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 6) Remove Nursery::chunkCountLimit() r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 7) Factor out a common assertion r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1568740 - (part 8) Add a test r=jonco 5 years ago Paul Bone [:pbone] 47 bytes, text/x-phabricator-request		Details \| Review

Paul Bone [:pbone]

Assignee

Description

•

5 years ago

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.

Paul Bone [:pbone]

Assignee

Comment 1

•

5 years ago

Attached file Bug 1568740 - (part 1) Remove Nursery::exists r=nbp — Details

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

Paul Bone [:pbone]

Assignee

Comment 2

•

5 years ago

Attached file Bug 1568740 - (part 2) Calculate the nursery's maximum size in bytes r=jonco — Details

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

Paul Bone [:pbone]

Assignee

Comment 3

•

5 years ago

Attached file Bug 1568740 - (part 3) Nursery::roundSize should be a static member r=jonco — Details

Depends on D39281

Paul Bone [:pbone]

Assignee

Comment 4

•

5 years ago

Attached file Bug 1568740 - (part 4) Use Nursery::roundSize() whenever setting the nursery size r=jonco — Details

Depends on D39282

Paul Bone [:pbone]

Assignee

Comment 5

•

5 years ago

Attached file Bug 1568740 - (part 5) Rename a variable r=jonco — Details

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

Depends on D39283

Paul Bone [:pbone]

Assignee

Comment 6

•

5 years ago

Attached file Bug 1568740 - (part 6) Remove Nursery::chunkCountLimit() r=jonco — Details

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

Depends on D39284

Jon Coppeard (:jonco)

Comment 7

•

5 years ago

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

Paul Bone [:pbone]

Assignee

Comment 8

•

5 years ago

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

Paul Bone [:pbone]

Assignee

Comment 9

•

5 years ago

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

Paul Bone [:pbone]

Assignee

Comment 10

•

5 years ago

Attached file Bug 1568740 - (part 7) Factor out a common assertion r=jonco — Details

Depends on D39285

Jon Coppeard (:jonco)

Comment 11

•

5 years ago

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

Paul Bone [:pbone]

Assignee

Updated

•

5 years ago

Depends on: 1569151

Paul Bone [:pbone]

Assignee

Updated

•

5 years ago

Depends on: 1569840

Paul Bone [:pbone]

Assignee

Comment 12

•

5 years ago

Attached file Bug 1568740 - (part 8) Add a test r=jonco — Details

Depends on D39473

Pulsebot

Comment 13

•

5 years ago

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

Cristian Brindusan [:cbrindusan]

Comment 14

•

5 years ago

bugherder

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

Status: ASSIGNED → RESOLVED

Closed: 5 years ago

status-firefox70: --- → fixed

Resolution: --- → FIXED

Alexandru Michis [:malexandru]

Comment 15

•

5 years ago

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?

Flags: needinfo?(pbone)

Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)

Updated

•

5 years ago

Regressions: 1571439

Alexandru Michis [:malexandru]

Updated

•

5 years ago

Regressions: 1567793

Pulsebot

Comment 16

•

5 years ago

Backout by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ab4193c798
Backed out changesets 7ec72ad52389 to 251f98554914 for causing bug 1571439

Pulsebot

Comment 17

•

5 years ago

Backout by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ac0a35be0d3
Backed out changeset d681969e4480 too , missed from previous commit

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

Comment 18

•

5 years ago

•

Edited

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

Status: RESOLVED → REOPENED

status-firefox70: fixed → ---

Resolution: FIXED → ---

Target Milestone: mozilla70 → ---

Paul Bone [:pbone]

Assignee

Updated

•

5 years ago

Depends on: 1567793

No longer regressions: 1567793

Pulsebot

Comment 19

•

5 years ago

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

Dorel Luca [:dluca]

Comment 20

•

5 years ago

bugherder

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

Status: REOPENED → RESOLVED

Closed: 5 years ago → 5 years ago

status-firefox71: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla71

Paul Bone [:pbone]

Assignee

Updated

•

5 years ago

No longer depends on: 1567793

Flags: needinfo?(pbone)

Paul Bone [:pbone]

Assignee

Updated

•

5 years ago

Regressions: 1584181

You need to log in before you can comment on or make changes to this bug.