782657 - Convert use of sigslot to multithreaded

Status: NEW

(Core :: WebRTC, defect)

Description

[Eric Rescorla (:ekr)]

We use sigslot to make mtransport and mediapipeline work. There is a multithreaded mode which is activated by a define. We need it. This is a build issue.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Perhaps more to the point (though a different issue), we need to either subset libjingle for m-c (so we get sigslot without everything), or add sigslot to chromium/ipc, or import it separately, or reimplement it.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Sigslot has been moved over, but we still need to decide to turn on the multi-threaded mode.  ekr, do we need multithreaded mode, or is it just a good idea, or does it not matter in our use?

Comment 3

[Eric Rescorla (:ekr)]

At minimum it's a good idea.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: switch all uses of sigslot to default to multithreaded (win32 already defaults that way)

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 690201 [details] [diff] [review]
switch all uses of sigslot to default to multithreaded (win32 already defaults that way)

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

This looks good for Mac and Linux. Does it force POSIX threads on Windows tho?

Comment 6

[Eric Rescorla (:ekr)]

Comment on attachment 690201 [details] [diff] [review]
switch all uses of sigslot to default to multithreaded (win32 already defaults that way)

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

This looks good for Mac and Linux. Does it force POSIX threads on Windows tho?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

No, it uses windows threading, but specifying this makes sure it selects non-single-threaded.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea2ccfd704e

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Backed out due to deadlocks (reported by ekr, I saw one on shutdown but didn't diagnose the cause):

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb5a5a91af6

Also, the patch had the wrong bug number (752657) so it's just as well ;-)

Marking leave-open so if first patch gets uplifted this won't get closed.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Not sure we even want this anymore...  blocking-

Comment 11

[Maire Reavy [:mreavy] Plz needinfo]

Byron: Do we still want this?  Or is it no longer relevant?  Or would it let us simplify other code eventually?

Comment 12

[Byron Campen [:bwc]]

It might be a necessary step toward simplifying some code, although it won't be sufficient.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Ok, so we should keep it (as it has a patch and may be a dependency of other simplification work).  Perhaps we should open a bug on reducing the number of cross-thread transfers, and have this a blocker or see-also for that.