Closed Bug 1061827 Opened 6 years ago Closed 6 years ago

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


(Firefox OS Graveyard :: NFC, defect)

Not set


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

2.1 S4 (12sep)
blocking-b2g -
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed


(Reporter: smaug, Assigned: tauzen)




(1 file, 3 obsolete files)
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: nobody → kmioduszewski
Refactored debug method in Nfc.js and remove the unnecessary JSON.stringify usage.
Attachment #8483723 - Flags: review?(allstars.chh)
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]

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)
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]

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) {

you can do: DEBUG && debug(...) which saves you the 2 extra lines.
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+

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 :)
Keywords: checkin-needed
Closed: 6 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)
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:, 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 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]:
Attachment #8484826 - Flags: approval-mozilla-aurora?
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+
You need to log in before you can comment on or make changes to this bug.