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

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

Firefox OS
NFC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: smaug, Assigned: tauzen)

Tracking

unspecified
2.1 S4 (12sep)
x86_64
Linux

Firefox Tracking Flags

(blocking-b2g:-, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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).
[Blocking Requested - why for this release]:
regression from Bug 1059102
blocking-b2g: --- → 2.1?
(Assignee)

Updated

4 years ago
Assignee: nobody → kmioduszewski
(Assignee)

Comment 2

4 years ago
Created attachment 8483723 [details] [diff] [review]
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.
Attachment #8483723 - Flags: review?(allstars.chh)
(Assignee)

Comment 3

4 years ago
Created attachment 8483730 [details] [diff] [review]
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?
Attachment #8483730 - Flags: review?(allstars.chh)
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
Attachment #8483723 - Flags: review?(allstars.chh)
(Assignee)

Comment 6

4 years ago
Created attachment 8484103 [details] [diff] [review]
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+.
Attachment #8483723 - Attachment is obsolete: true
Attachment #8483730 - Attachment is obsolete: true
Attachment #8483730 - Flags: review?(allstars.chh)
Attachment #8484103 - Flags: review?(allstars.chh)
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.
Attachment #8484103 - Flags: review?(allstars.chh) → review+
Nit: instead of:
if (DEBUG) {
 debug(...);
}

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

Comment 9

4 years ago
Created attachment 8484826 [details] [diff] [review]
(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.
Attachment #8484103 - Attachment is obsolete: true
Attachment #8484826 - Flags: review+
(Assignee)

Comment 10

4 years ago
Try results: https://tbpl.mozilla.org/?tree=Try&rev=eed0af820f1f
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0bb6b880744e
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S4 (12sep)
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?
Flags: needinfo?(kmioduszewski)
(Assignee)

Comment 14

4 years ago
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.
Flags: needinfo?(kmioduszewski) → needinfo?(allstars.chh)
Although no user impact I'd still prefer to uplift this as Olli said this will have impact in memory usage.
Flags: needinfo?(allstars.chh)
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.
(Assignee)

Comment 17

4 years ago
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
Attachment #8484826 - Flags: approval-mozilla-aurora?

Updated

4 years ago
blocking-b2g: 2.1? → -
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.
Attachment #8484826 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ef9fa6a0cff
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed
You need to log in before you can comment on or make changes to this bug.