Closed Bug 1197019 Opened 7 years ago Closed 7 years ago

Update Camera to be ready for L10n API v3

Categories

(Firefox OS Graveyard :: Gaia::Camera, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

46 bytes, text/x-github-pull-request
wilsonpage
: review+
Details | Review
No description provided.
Attached file pull request
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8650791 [details] [review]
pull request

Andrew, can you review this patch for me please?

Context: We're working on a next-generation l10n library that, among other goodies, is fully asynchronous. To prepare for that, we're moving away from all synchronous mozL10n.get calls.

On top of that we're migrating our date/time formatting to use Intl API - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl
Attachment #8650791 - Flags: review?(aosmond)
Comment on attachment 8650791 [details] [review]
pull request

Trying a different reviewer.
Attachment #8650791 - Flags: review?(aosmond) → review?(dflanagan)
Comment on attachment 8650791 [details] [review]
pull request

I'm not the right reviewer for this, either. I think you need Wilson or Justin for this one.

Wilson, Justin: can one of you take this review, please?
Flags: needinfo?(wilsonpage)
Flags: needinfo?(jdarcangelo)
Attachment #8650791 - Flags: review?(dflanagan)
Comment on attachment 8650791 [details] [review]
pull request

Some nit/questions inline.
Flags: needinfo?(wilsonpage)
Comment on attachment 8650791 [details] [review]
pull request

updated the patch, rerequesting review.
Flags: needinfo?(jdarcangelo)
Attachment #8650791 - Flags: review?(wilsonpage)
Comment on attachment 8650791 [details] [review]
pull request

Just like to see some performance numbers from Flame to clarify that this approach isn't regressing startup.

BTW how much is l10n/l20.js expected to add to an app's startup time?
Attachment #8650791 - Flags: review?(wilsonpage)
Yeah! I didn't forget - I just updated to node 4 and now raptor doesn't work ;) (bug 1202921).

wrt. expectations - so far we see 0 to -100ms performance difference between l10n.js/l20n.js and 0 to +100kb of memory.
Comment on attachment 8650791 [details] [review]
pull request

Ok, I learned how to use nvm to manage my node versions.

Flame-kk from today with 31 runs

master:
| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| navigationLoaded      | 4096.226 | 4104   | 3420   | 4249   | 134.839 | 4222.900 |
| navigationInteractive | 4096.613 | 4104   | 3420   | 4249   | 134.866 | 4222.900 |
| visuallyLoaded        | 4327.581 | 4329   | 3663   | 4476   | 134.869 | 4476     |
| contentInteractive    | 5245.548 | 5247   | 4558   | 5390   | 137.387 | 5384.450 |
| fullyLoaded           | 5245.903 | 5248   | 4558   | 5391   | 137.490 | 5384.450 |
| pss                   | 24.454   | 24.451 | 24.372 | 24.604 | 0.043   | 24.523   |
| uss                   | 19.469   | 19.465 | 19.387 | 19.609 | 0.041   | 19.538   |
| rss                   | 41.852   | 41.852 | 41.770 | 42.016 | 0.045   | 41.921   |

patch:
| Metric                | Mean     | Median | Min    | Max    | StdDev  | p95      |
| --------------------- | -------- | ------ | ------ | ------ | ------- | -------- |
| navigationLoaded      | 3996.871 | 4011   | 3456   | 4153   | 115.768 | 4094     |
| navigationInteractive | 3997.290 | 4011   | 3457   | 4154   | 115.684 | 4095     |
| visuallyLoaded        | 4357     | 4370   | 3807   | 4492   | 119.202 | 4473.950 |
| contentInteractive    | 5173.968 | 5202   | 4624   | 5332   | 118.466 | 5308.550 |
| fullyLoaded           | 5174.452 | 5202   | 4625   | 5332   | 118.480 | 5308.600 |
| uss                   | 19.521   | 19.543 | 19.402 | 19.660 | 0.067   | 19.617   |
| rss                   | 41.890   | 41.922 | 41.758 | 42.039 | 0.075   | 41.992   |
| pss                   | 24.504   | 24.529 | 24.380 | 24.647 | 0.070   | 24.603   |
Attachment #8650791 - Flags: review?(wilsonpage)
Attachment #8650791 - Flags: review?(wilsonpage) → review+
Commit: https://github.com/mozilla-b2g/gaia/commit/eb9d29d5af8ff5b2345b77e1ab30ab4ad4987fe9

Thanks!
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Nice wins visible on Aries and Flame in performance, but there seem to be a regression in memory. I'll investigate on Monday.
I can't reproduce any memory regression for this patch.

The raptor numbers looked at from the perspective of three days look better.

On Flame, the USS before landing (25th 10:10am mark) was around 18.9mb. The mark right after landing (13:13) jumped to 20.4 which is worrying, but after that the numbers drop over the weekend to 18.4-18.1mb. That makes me feel that the first result was likely a fluke.

I also tested Z3 locally with and without the patch again and got the same results as in comment 9 - delta within 60kb, stdev within 60kb.

Wrt. performance, it looks better.

On Flame, the navigationLoaded seems to drop from ~5.4s to 5.2s while fullyLoaded from 7.7s, to 7.5s with similar win on Z3 where fullyLoaded dropped from 3.2 to 3.0s so <200ms win.

If you see any regressions that I didn't detect, please, let me know.
You need to log in before you can comment on or make changes to this bug.