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

Status: RESOLVED FIXED

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

Description

[André Bargull [:anba]]

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.

Comment 1

[Jon Coppeard (:jonco)]

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

Comment 2

[Jon Coppeard (:jonco)]

Attachment: Bug 1591711 - Fix incorrect calculation of non-incremental GC trigger r?sfink

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38b7d058a18f
Fix incorrect calculation of non-incremental GC trigger r=allstarschh

Comment 4

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/38b7d058a18f

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[Jon Coppeard (:jonco)]

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

Comment 7

[Pascal Chevrel:pascalc (PTO until Sept 2)]

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.

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8cafc932a001