1173802 - Improve NFC I/O code

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

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

NFC messages are send and received on the I/O thread, processed on the NFC thread, and the actual DOM code runs on the main thread. Messages passing between these threads are often send via the main thread, even though main only forwards to the next thread.

The NFC I/O code can be made cleaner and faster by exchanging messages directly between NFC thread and I/O thread, and removing the extra hop through main.

See also a similar improvement for RIL in bug 1171994

Comment 1

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

Attachment: [01] Bug 1173802: Move NFC socket and messaging code into |NfcConsumer|

Comment 2

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

Attachment: [02] Bug 1173802: Run |NfcConsumer| on NFC thread

Comment 3

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

Attachment: [03] Bug 1173802: Clean up |NfcEventDispatcher|

Comment 4

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

Attachment: [04] Bug 1173802: Replace |assertIsNfcServiceThread| with thread-safe checks

Comment 5

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

Attachment: [05] Bug 1173802: Store global NFC service in ref-counted variable

Comment 6

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

Hi Yoshi,

I know you have a tight schedule, but the patch set isn't urgent. Take your time with the review.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Thanks, I'll be quite busy recently,
so I expect to review these after work week. i.e. in July.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8621479 [details] [diff] [review]
[01] Bug 1173802: Move NFC socket and messaging code into |NfcConsumer|

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

::: dom/nfc/gonk/NfcService.cpp
@@ +16,4 @@
>  #include "mozilla/ipc/NfcConnector.h"
> +#include "mozilla/ipc/StreamSocket.h"
> +#include "mozilla/ipc/StreamSocketConsumer.h"
> +#include "mozilla/ModuleUtils.h"

Are you trying yo sort this alphabetically ?
If yes then this should be moved to after mozilla/ClearOnShutdown.h

@@ +20,3 @@
>  #include "mozilla/unused.h"
> +#include "NfcMessageHandler.h"
> +#include "NfcOptions.h"

ditto

@@ +434,5 @@
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  switch (aIndex) {
> +    case LISTEN_SOCKET: {
> +        nsCString value("nfcd:-a ");

indent, should be 2 spaces.

Comment 9

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

> >  #include "mozilla/ipc/NfcConnector.h"
> > +#include "mozilla/ipc/StreamSocket.h"
> > +#include "mozilla/ipc/StreamSocketConsumer.h"
> > +#include "mozilla/ModuleUtils.h"
> 
> Are you trying yo sort this alphabetically ?
> If yes then this should be moved to after mozilla/ClearOnShutdown.h

It comes after mozilla/ClearOnShutdown. (?)

> 
> @@ +434,5 @@
> > +  MOZ_ASSERT(NS_IsMainThread());
> > +
> > +  switch (aIndex) {
> > +    case LISTEN_SOCKET: {
> > +        nsCString value("nfcd:-a ");
> 
> indent, should be 2 spaces.

OK.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8621479 [details] [diff] [review]
[01] Bug 1173802: Move NFC socket and messaging code into |NfcConsumer|

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

::: dom/nfc/gonk/NfcService.cpp
@@ +16,4 @@
>  #include "mozilla/ipc/NfcConnector.h"
> +#include "mozilla/ipc/StreamSocket.h"
> +#include "mozilla/ipc/StreamSocketConsumer.h"
> +#include "mozilla/ModuleUtils.h"

Sorry I mean 
#include "mozilla/ModuleUtils.h" should be moved to line 11.

Comment 11

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

I still don't get it. 'd' and 'i' come before 'm'. AFAICT the coding style doesn't require treating directories differently.

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8cbb50d171d

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #11)
> I still don't get it. 'd' and 'i' come before 'm'. AFAICT the coding style
> doesn't require treating directories differently.

okay then fine by me
I thought we sort it by ascii code.

Comment 14

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

Attachment: [01] Bug 1173802: Move NFC socket and messaging code into |NfcConsumer| (v2)

Changes since v1:

  - rebased onto m-c
  - fixed coding style according to review

Comment 15

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

Attachment: [02] Bug 1173802: Run |NfcConsumer| on NFC thread (v2)

Changes since v1:

  - rebased onto m-c

Comment 16

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

Attachment: [03] Bug 1173802: Clean up |NfcEventDispatcher| (v2)

Changes since v1:

  - rebased onto m-c

Comment 17

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

Attachment: [04] Bug 1173802: Replace |assertIsNfcServiceThread| with thread-safe checks (v2)

Changes since v1:

  - rebased onto m-c

Comment 18

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

Attachment: [05] Bug 1173802: Store global NFC service in ref-counted variable (v2)

Changes since v1:

  - rebased onto m-c

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/d59e98209a18
https://hg.mozilla.org/integration/b2g-inbound/rev/d732c4e52cf4
https://hg.mozilla.org/integration/b2g-inbound/rev/5c029f9eaf54
https://hg.mozilla.org/integration/b2g-inbound/rev/3037c7071f0b
https://hg.mozilla.org/integration/b2g-inbound/rev/c93b6381b8cd

Comment 20

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

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=c93b6381b8cd

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/d59e98209a18
https://hg.mozilla.org/mozilla-central/rev/d732c4e52cf4
https://hg.mozilla.org/mozilla-central/rev/5c029f9eaf54
https://hg.mozilla.org/mozilla-central/rev/3037c7071f0b
https://hg.mozilla.org/mozilla-central/rev/c93b6381b8cd