1410123 - Atoms bitmap is only updated when we collect the atoms zone

Status: REOPENED

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

Description

[Jon Coppeard (:jonco)]

As per bug 1324002 comment 28, we should udpate the atoms bitmaps in every collection to make the approximations more accurate and enable us to free more unused atoms sooner when collect the atoms zone in a zone GC.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1410123-always-update-atoms-bitmap

Here's a patch that always collects the atoms zone, but only sweeps it if it was actually scheduled.  What this lets us do is to update the atoms bitmaps for collected zones in every GC, even if we didn't schedule the atoms zone.  The per-zone atoms bitmaps are an overapproximation to the set of atoms referenced from that zone, and this update step allows us to refine this with information about which atoms were actually marked.  Then when we later sweep the atoms zone this is taken into account and potentially lets us free more atoms.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8921531 [details] [diff] [review]
bug1410123-always-update-atoms-bitmap

Review of attachment 8921531 [details] [diff] [review]:
-----------------------------------------------------------------

The patch seems fine, but for my understanding, if you have multiple sweep groups which one of them is the atoms zone in? The first one, I guess?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
If the atoms zone is being collected it will be the single zone in the last group, since every zone will adds edge to the atoms zone in Zone::findOutgoingEdges().

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c36f41ed77c
Update atom marking bitmaps on every collection r=sfink

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/2c36f41ed77c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Backed out for making Windows mochitest-gl leak the world about 30-40% of the time.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bfaea0c03ec2b00ece8f769720ff816d6832ca9

https://treeherder.mozilla.org/logviewer.html#?job_id=139695292&repo=mozilla-inbound

Comment 7

[Ionuț Goldan [:igoldan]]

When this first landed, we noticed these performance regressions:

== Change summary for alert #10164 (as of October 25 2017 09:31 UTC) ==

Regressions:

 27%  tp6_youtube summary windows7-32 opt e10s     296.73 -> 375.79
 23%  tp6_youtube_heavy summary windows10-64 opt e10s274.88 -> 338.50
 22%  tp6_youtube_heavy summary windows7-32 opt e10s288.88 -> 351.69

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10164

Comment 8

[Ionuț Goldan [:igoldan]]

Preliminary results after the backout show that the regression was removed.

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/0d1e55d87931

Comment 10

[Ionuț Goldan [:igoldan]]

I confirm the backout canceled the perf regressions:

== Change summary for alert #10190 (as of October 26 2017 10:48 UTC) ==

Improvements:

 22%  tp6_youtube_heavy summary windows7-32 opt e10s     342.90 -> 266.12
 16%  tp6_youtube_heavy summary linux64 opt e10s         308.19 -> 258.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10190

Comment 11

[Intermittent Failures Robot]

13 failures in 912 pushes (0.014 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* mozilla-inbound: 11
* autoland: 2

Platform breakdown:
* windows10-64: 7
* windows7-32: 3
* windows10-64-stylo-disabled: 2
* windows7-32-stylo-disabled: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1410123&startday=2017-10-23&endday=2017-10-29&tree=all

Comment 12

[Matthew Gaudet (he/him) [:mgaudet]]

(This bounced and was never followed up -- marking as Incomplete)

Comment 13

[Jon Coppeard (:jonco)]

We should look for a way to do this if we can without regressing performance.

Comment 14

[Matthew Gaudet (he/him) [:mgaudet]]

Thank you for re-opening this! Rooting this to get it out of future triage lists.