Closed
Bug 1168806
Opened 9 years ago
Closed 9 years ago
Configurable socket threads
Categories
(Firefox OS Graveyard :: General, defect)
Tracking
(firefox41 fixed)
RESOLVED
FIXED
2.2 S14 (12june)
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: tzimmermann, Assigned: tzimmermann)
References
Details
Attachments
(3 files)
31.18 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
73.34 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
38.12 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
The socket I/O classes currently transfer data between main thread and I/O thread. The threads should be configurable. This might allow for more flexible thread setups, like doing I/O on a worker thread (e.g., for RIL).
Assignee | ||
Comment 1•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=509c5acd04f4
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8613437 -
Flags: review?(kyle)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8613438 -
Flags: review?(kyle)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8613439 -
Flags: review?(kyle)
Comment 5•9 years ago
|
||
Comment on attachment 8613437 [details] [diff] [review] [01] Bug 1168806: Configurable I/O thread for socket IPC classes Review of attachment 8613437 [details] [diff] [review]: ----------------------------------------------------------------- ::: ipc/unixsocket/StreamSocket.cpp @@ +523,5 @@ > } > > nsresult > +StreamSocket::Connect(UnixSocketConnector* aConnector, int aDelayMs, > + MessageLoop* aIOLoop) Nit that happens a lot of places around this patch: Sometimes we assert on raw pointers, sometimes we don't. Do we want to assert on aConnector/aIOLoop/other raw pointers in all of the places these show up?
Attachment #8613437 -
Flags: review?(kyle) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8613438 [details] [diff] [review] [02] Bug 1168806: Configurable consumer thread for socket IPC classes Review of attachment 8613438 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothSocket.cpp @@ +808,5 @@ > } > > void > BluetoothSocket::OnConnectSuccess() > { Now that we have consumer threads, will we ever want to start/stop threads on the main thread? If not, should we check for that?
Attachment #8613438 -
Flags: review?(kyle) → review+
Updated•9 years ago
|
Attachment #8613439 -
Flags: review?(kyle) → review+
Assignee | ||
Comment 7•9 years ago
|
||
>
> Nit that happens a lot of places around this patch: Sometimes we assert on
> raw pointers, sometimes we don't. Do we want to assert on
> aConnector/aIOLoop/other raw pointers in all of the places these show up?
Too few exceptions and you run into bugs, too many and the code becomes unreadable. :/
In general, my policy is to put asserts into the methods of a public interface, such as |StreamSocket|, and constructors, but not internal methods. So once something is set up, I expect it to work. There might be exceptions to the rule, though.
I'm open to any suggestions.
Assignee | ||
Comment 8•9 years ago
|
||
> ::: dom/bluetooth/bluedroid/BluetoothSocket.cpp
> @@ +808,5 @@
> > }
> >
> > void
> > BluetoothSocket::OnConnectSuccess()
> > {
>
> Now that we have consumer threads, will we ever want to start/stop threads
> on the main thread? If not, should we check for that?
The patches just generalize the code we already have. I don't think that the socket classes should do thread management. All threads are expected to be around while an instance is in use.
The 'consumer thread' is actually a producer and consumer. I couldn't come up with a better name than 'consumer'. It's what the main thread originally has been used for. That means that a consumer thread also has to open/close, construct/destruct its instances of a socket class.
For an error check: If there's a way to get a message when a thread gets deleted, we could let the socket classes register themselves for this message and then fail when it happens.
https://hg.mozilla.org/integration/b2g-inbound/rev/ef212da75478 https://hg.mozilla.org/integration/b2g-inbound/rev/075e96a57701 https://hg.mozilla.org/integration/b2g-inbound/rev/fd7c97d41cf3
Assignee | ||
Comment 10•9 years ago
|
||
Thanks! https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=fd7c97d41cf3
https://hg.mozilla.org/mozilla-central/rev/ef212da75478 https://hg.mozilla.org/mozilla-central/rev/075e96a57701 https://hg.mozilla.org/mozilla-central/rev/fd7c97d41cf3
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
You need to log in
before you can comment on or make changes to this bug.
Description
•