1061827 - Nfc.js serializes all the messages, even in release mode

Status: RESOLVED FIXED

(Firefox OS Graveyard :: NFC, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/dom/nfc/gonk/Nfc.js?rev=4cfa5c8e0ad0#414
That JSON.stringify(message) is totally useless if debug won't do anything.
And JSON.stringify(message) can be rather heavy and certainly creates some garbage (which gc then collects).

Comment 1

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

[Blocking Requested - why for this release]:
regression from Bug 1059102

Comment 2

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 0001-Bug-1061827-Part-1-Nfc.js-serializes-all-the-message.patch

Refactored debug method in Nfc.js and remove the unnecessary JSON.stringify usage.

Comment 3

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 0002-Bug-1061827-Part-2-Fixes-for-NfcContentHelper.js-r-a.patch

I've checked also NfcContentHelper.js and it also contained similar unnecessary JSON.stringify usage. This patch fixes it. 

Should I merge it with part 1 and submit one patch only?

Comment 4

[Krzysztof Mioduszewski[:tauzen]]

try-run results: https://tbpl.mozilla.org/?tree=Try&rev=4a2f532cecbe

Comment 5

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

Comment on attachment 8483723 [details] [diff] [review]
0001-Bug-1061827-Part-1-Nfc.js-serializes-all-the-message.patch

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

I'd prefer we check if JSON.stringify is really neccesary in our code, 
and if it is, we use DEBUG to wrap it

if (DEBUG) {
  debug(" this obj is " + JSON.stringify(obj));
}

and merge two patches since they are doing the same thing

Comment 6

[Krzysztof Mioduszewski[:tauzen]]

Attachment: 0001-Bug-1061827-Nfc.js-serializes-all-the-messages-even-.patch

Changes requested by Yoshi. I'll test this on a device once my build will finish. I can not run marionettes tests because of Bug 1062786. Will push this to try-server if it gets r+.

Comment 7

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

Comment on attachment 8484103 [details] [diff] [review]
0001-Bug-1061827-Nfc.js-serializes-all-the-messages-even-.patch

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

::: dom/nfc/gonk/Nfc.js
@@ +416,5 @@
>      let message = Cu.cloneInto(event, this);
> +    if (DEBUG) {
> +      debug("Received message from NFC Service: " + JSON.stringify(message));
> +    }
> +    

nit, trailing space.

Comment 8

[[:fabrice] Fabrice Desré]

Nit: instead of:
if (DEBUG) {
 debug(...);
}

you can do: DEBUG && debug(...) which saves you the 2 extra lines.

Comment 9

[Krzysztof Mioduszewski[:tauzen]]

Attachment: (v1.1) 0001-Bug-1061827-Nfc.js-serializes-all-the-messages-even-.patch

Thanks Yoshi and Fabrice for your comments, I fixed both of them in this version.

Setting r+ myself, since Yoshi already r+'ed the previous version in comment 7. Will ask for checkin once the try build will be over.

Comment 10

[Krzysztof Mioduszewski[:tauzen]]

Try results: https://tbpl.mozilla.org/?tree=Try&rev=eed0af820f1f

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0bb6b880744e

Keep in mind that in general, your commit message should be summarizing what the patch is doing, not restating the problem it's solving. Thanks :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0bb6b880744e

Comment 13

[Dietrich Ayala (:dietrich)]

Hi Krzysztof, we're reviewing this bug in triage, and are unclear on the user impact of shipping this. Comment #1 says it's a regression. Is it a regression from 2.0?

Comment 14

[Krzysztof Mioduszewski[:tauzen]]

One of the debug messages pointed out by Olli was introduced in Bug 1059102, this is why Yoshi marked this as a regression from this bug, but this is a 2.1 bug. This patch removes the unnecessary JSON.stringify usage in debug messages so it might lower the memory usage. There is no visible user impact. From my perspective it is not essential to ship this, but it would be good to ask Yoshi for his opinion.

Comment 15

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

Although no user impact I'd still prefer to uplift this as Olli said this will have impact in memory usage.

Comment 16

[Dietrich Ayala (:dietrich)]

Cool, thanks for the explanation. Blocker criteria means we don't hold back the release for perf improvements: https://wiki.mozilla.org/B2G/Triage#Blocker_Triage_Guidelines, so I expect this will be minused in triage.

However, we definitely want to take this if it's low risk and a memory win. Ask for approval on the patch, and the release sheriffs (fabrice/bhavana) will evaluate it.

For clarification: blocking- combined with approval+ means that if this patch causes problems after landing, we'd back it out and ship anyway.

Comment 17

[Krzysztof Mioduszewski[:tauzen]]

Comment on attachment 8484826 [details] [diff] [review]
(v1.1) 0001-Bug-1061827-Nfc.js-serializes-all-the-messages-even-.patch

Approval Request Comment
[Feature/regressing bug #]:
Partially Bug 1059102, see comment 14.
[User impact if declined]:
Minor, memory usage will be higher.
[Describe test coverage new/current, TBPL]:
Manually tested, marionette web-api tests.
[Risks and why]:
Low, because this is only debug message change. 
[String/UUID change made/needed]:
None

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 8484826 [details] [diff] [review]
(v1.1) 0001-Bug-1061827-Nfc.js-serializes-all-the-messages-even-.patch

Not blocking for the release but approving uplift given the low risk here.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ef9fa6a0cff