1119550 - Stop using volatile for synchronization

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Or rather, *trying* to use volatile for synchronization: I doubt it's actually doing anything at all.

Comment 1

[Terrence Cole [:terrence]]

Attachment: nonvolatile_interFrameGC-v1.diff

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=763a73d2a75c
Try run looks dirty, but is pretty much the same as the tip I pushed against:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8439a009837d

https://hg.mozilla.org/integration/mozilla-inbound/rev/e98f0dc6b1c3

Comment 3

[Terrence Cole [:terrence]]

Attachment: eliminate_volatile_on_majorGCRequested-v0.diff

Well, the world didn't end, so let's do the next one.

For majorGCRequested, we need to have a valid majorGCRequestReason in both threads, so we'd nominally need at least ReleaseAcquire semantics (e.g. to transfer ownership of the reason). However, there's not really any good reason to use two variables when we can just signal not-requested with NO_REASON. If we reduce the information we need to reliably transfer to a single word, we can use Relaxed ordering and be certain that we're not regressing performance.

The attached does this for majorGCRequested and the same for minorGCRequested to maintain the nice symmetry.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67969d5e2e56

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8550761 [details] [diff] [review]
eliminate_volatile_on_majorGCRequested-v0.diff

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

Using a single field for these is much nicer!

I'm a little cautious about using relaxed atomics, but I think it is ok here.  We're not trying to synchronise, and we don't care if the main thread sees the change immediately.

Comment 5

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67969d5e2e56
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee3e8054b72

Only one more after this.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6ee3e8054b72

Comment 7

[Terrence Cole [:terrence]]

The "one-more" is the volatile on the mark bitmap. That's not something we can replace with atomics sanely, so I'm going to go ahead and close this bug.

Comment 8

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.