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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Luke Wagner [:luke]]

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.

Comment 1

[Luke Wagner [:luke]]

Attachment: no-lock-in-dtor

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(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 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8963148 [details] [diff] [review]
no-lock-in-dtor

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

LGTM!

Comment 4

[Luke Wagner [:luke]]

Attachment: simplify-sig-id-set

Follow-up simplification allowed by previous patch.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

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?

Comment 6

[Luke Wagner [:luke]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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)

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9b7418660fe2
https://hg.mozilla.org/mozilla-central/rev/fbcb35097910