Closed Bug 1449189 Opened Last year Closed Last year

Does ~ExclusiveData() really need to take a lock?

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files)

This bug is to ask whether we can revert bug 1260891.  The reasoning there makes conservative sense: since the destructor is about to read shared values, we need to ensure previous read happen-before the destructor.  However, I think we have this happens-before relationship even without acquiring the lock: ~ExclusiveData must not race with any other use of the object since, if the other use loses the race, it will be using a dead object.  This implies that the user of ExclusiveData must externally ensure that destructions happens-after all previous uses.  Practically speaking, this will be ensured by any extant threads or tasks on other threads having been joined/completed/canceled by the time the ExclusiveData is destroyed.
Priority: -- → P3
Attached patch no-lock-in-dtorSplinter Review
Assignee: nobody → luke
Attachment #8963148 - Flags: review?(nfitzgerald)
(In reply to Luke Wagner [:luke] from comment #0)
> This bug is to ask whether we can revert bug 1260891.  The reasoning there
> makes conservative sense: since the destructor is about to read shared
> values, we need to ensure previous read happen-before the destructor. 
> However, I think we have this happens-before relationship even without
> acquiring the lock: ~ExclusiveData must not race with any other use of the
> object since, if the other use loses the race, it will be using a dead
> object.  This implies that the user of ExclusiveData must externally ensure
> that destructions happens-after all previous uses.  Practically speaking,
> this will be ensured by any extant threads or tasks on other threads having
> been joined/completed/canceled by the time the ExclusiveData is destroyed.

This makes perfect sense. Thanks!

Taking a look at the patch now.
Comment on attachment 8963148 [details] [diff] [review]
no-lock-in-dtor

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

LGTM!
Attachment #8963148 - Flags: review?(nfitzgerald) → review+
Follow-up simplification allowed by previous patch.
Attachment #8963723 - Flags: review?(bbouvier)
Comment on attachment 8963723 [details] [diff] [review]
simplify-sig-id-set

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

Looks good! Any chance we could put the SigSet in WasmProcess too, or is that too much hassle?
Attachment #8963723 - Flags: review?(bbouvier) → review+
(In reply to Benjamin Bouvier [:bbouvier] from comment #5)
Conceptually that sounds nice, but it would increase the scope of SigIdSet from being a WasmInstance.cpp-internal detail to a whole-wasm/-visible thing, so unless there was some more tangible benefit for colocating all the process-lifetime things, I'd default to keeping it where it is.
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7418660fe2
Don't lock in ~ExclusiveData (r=fitzgen)
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbcb35097910
Baldr: remove ExclusiveData indirection (r=bbouvier)
https://hg.mozilla.org/mozilla-central/rev/9b7418660fe2
https://hg.mozilla.org/mozilla-central/rev/fbcb35097910
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.