Closed Bug 1591711 Opened 9 months ago Closed 9 months ago

GCRuntime::checkHeapThreshold unlikely to trigger non-incremental GCs, because it multiplies thresholdBytes with itself

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- wontfix
firefox71 --- fixed
firefox72 --- fixed

People

(Reporter: anba, Assigned: jonco)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/7536d7f480a7f18c941a590a2d4c5119d9f52770/js/src/gc/GC.cpp#3021-3026:

  size_t niThreshold = thresholdBytes * tunables.nonIncrementalFactor();
  if (usedBytes >= thresholdBytes * niThreshold) {
    // We have passed the non-incremental threshold: immediately trigger a
    // non-incremental GC.
    return TriggerResult{TriggerKind::NonIncremental, usedBytes, niThreshold};
  }

thresholdBytes * niThreshold in the if-condition looks wrong, because that's thresholdBytes * thresholdBytes * tunables.nonIncrementalFactor(). The correct condition should most likely be usedBytes >= niThreshold.

Blocks: 1585536

Nice catch. I broke this while refactoring in bug 1570905.

Assignee: nobody → jcoppeard
Regressed by: 1570905

Fix a mistake calculating the heap size to compare against. It's unlikely this bug had much effect because whether to perform an incremental or non-incremental collection is decided when we start the collection in GCRuntime::budgetIncrementalGC().

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38b7d058a18f
Fix incorrect calculation of non-incremental GC trigger r=allstarschh
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Sounds like this fix can ride the trains given comment 2, but feel free to nominate for Beta approval if you feel otherwise.

Comment on attachment 9104683 [details]
Bug 1591711 - Fix incorrect calculation of non-incremental GC trigger r?sfink

Beta/Release Uplift Approval Request

  • User impact if declined: This change seems to have reduced the number of GC resets (From ~0.38% to ~0.1% on beta) The occurrence is still very low but this is such a simple fix it may be worth taking anyway. The user impact would be slightly less chance of jank due to GC.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a very simple fix that returns a GC trigger calculation to its original state.
  • String changes made/needed: None
Attachment #9104683 - Flags: approval-mozilla-beta?

Comment on attachment 9104683 [details]
Bug 1591711 - Fix incorrect calculation of non-incremental GC trigger r?sfink

Low risk, has tests and was on beta for a week, uplift approved for 71 beta 8, thanks.

Attachment #9104683 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.