932728 - [DBus] Run DBus client on I/O thread

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

If we can run the DBus code on the I/O thread, we can save the resources of the extra DBus thread and simplify the code base.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] 932728: Don't inherit DBusWatcher from RawDBusConnection

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 932728: Open only one connection to DBus

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 932728: Use private DBus connection

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 932728: Run DBus on the I/O thread

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi,

These patches worked nicely when I tested them, but I'd expect they might contain subtle bugs somewhere.

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

BTW, you don't need to go through fb? then r? if things are mostly ready, you can just r? things and save yourself having to ask me to go back through after I fb+ stuff.

Comment 7

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 824577 [details] [diff] [review]
[04] Bug 932728: Run DBus on the I/O thread

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

Wow. So clean. :D

Comment 8

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 824573 [details] [diff] [review]
[01] 932728: Don't inherit DBusWatcher from RawDBusConnection

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

The modification of this patch seems to be mostly covered by the patch of bug 931038. The patch looks good, but please remember to rebase before checking in.

Comment 9

[Eric Chou [:ericchou] [:echou]]

Hey Thomas,

Sorry for the late reply. I'll review rest patches on Tuesday since I'll be out-of-office on Monday.

Comment 10

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 824574 [details] [diff] [review]
[02] Bug 932728: Open only one connection to DBus

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

Looks good.

Comment 11

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 824576 [details] [diff] [review]
[03] Bug 932728: Use private DBus connection

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

Looks good.

Comment 12

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 824577 [details] [diff] [review]
[04] Bug 932728: Run DBus on the I/O thread

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

To be honest, I'm not familiar with previous implementation of DBusThread.cpp. So r+ since Kyle is happy with this. :)

Thanks for cleaning this up!

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

No complains? :) I though that I should get some feedback first, because of the size of the patch set.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #8)
> Comment on attachment 824573 [details] [diff] [review]
> [01] 932728: Don't inherit DBusWatcher from RawDBusConnection
> 
> Review of attachment 824573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The modification of this patch seems to be mostly covered by the patch of
> bug 931038. The patch looks good, but please remember to rebase before
> checking in.

I uploaded the wrong patch file. :( But I'll supply the correct one for the actual review.

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] 932728: Don't inherit DBusWatcher from RawDBusConnection

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 932728: Run DBus on the I/O thread (v2)

Only rebased.

Comment 17

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> No complains? :) I though that I should get some feedback first, because of
> the size of the patch set.

Yeah, just throwing things up for review first is fine. I usually only fb? when I have something half done and want someone looking at the work in progress but the patch itself definitely couldn't land.

Comment 18

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 824574 [details] [diff] [review]
[02] Bug 932728: Open only one connection to DBus

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

r=me with nits addressed.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +1654,5 @@
>    // Normally we'll receive the signal 'AdapterAdded' for the default
>    // adapter from the DBus daemon during start up. If we restart after
>    // a crash, the default adapter might already be available, so we ask
>    // the daemon explicitly here.
> +  bool success = connection->SendWithReply(OnDefaultAdapterReply, nullptr,

super-nit: please line up.

Comment 19

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 932728: Open only one connection to DBus

Rebased on latest Bluetooth code.

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 932728: Open only one connection to DBus (v3)

Rebased on top of latest Bluetooth code.

Comment 21

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Finally :)

https://hg.mozilla.org/integration/b2g-inbound/rev/b4b1686e4db2
https://hg.mozilla.org/integration/b2g-inbound/rev/905ce734b772
https://hg.mozilla.org/integration/b2g-inbound/rev/93089287545b
https://hg.mozilla.org/integration/b2g-inbound/rev/0ddb34970ecb

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=0ddb34970ecb

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b4b1686e4db2
https://hg.mozilla.org/mozilla-central/rev/905ce734b772
https://hg.mozilla.org/mozilla-central/rev/93089287545b
https://hg.mozilla.org/mozilla-central/rev/0ddb34970ecb