Closed Bug 1210720 Opened 10 years ago Closed 10 years ago

Migrate Music to l20n-service & l20n-client

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Music
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

Attachments

(1 file)

[gaia] stasm:1210720-music-l20n-bridge > mozilla-b2g:master
46 bytes, text/x-github-pull-request
justindarc
: review+
Details | Review
The new Music app is a perfect candidate to test the bridge-enabled version of L20n. This bug will track the migration progress.
A quick teaser, using raptor data from bug 1210691 master: [gaia] raptor test coldlaunch --app music --output quiet --runs 10 | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | -------- | ------ | ------ | ------- | ------ | | navigationLoaded | 3507 | 3522.500 | 3002 | 3725 | 182.579 | 3725 | | navigationInteractive | 3526.500 | 3540.500 | 3023 | 3745 | 181.933 | 3745 | | visuallyLoaded | 4413.200 | 4387 | 4066 | 4650 | 147.380 | 4650 | | contentInteractive | 4414 | 4387 | 4067 | 4650 | 147.673 | 4650 | | fullyLoaded | 4414.500 | 4387.500 | 4067 | 4651 | 147.772 | 4651 | | uss | 20.980 | 20.713 | 20.297 | 22.074 | 0.640 | 22.074 | | pss | 25.168 | 24.890 | 24.479 | 26.291 | 0.653 | 26.291 | | rss | 43.452 | 43.156 | 42.758 | 44.617 | 0.671 | 44.617 | wip with all scripts in <head> and deferred [gaia] raptor test coldlaunch --app music --output quiet --runs 10 | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | -------- | ------ | ------ | ------- | ------ | | navigationLoaded | 3375.500 | 3422.500 | 2865 | 3547 | 182.160 | 3547 | | navigationInteractive | 3394.800 | 3441 | 2884 | 3564 | 182.205 | 3564 | | visuallyLoaded | 4218 | 4272 | 3758 | 4366 | 168.853 | 4366 | | contentInteractive | 4218.400 | 4273 | 3759 | 4366 | 168.722 | 4366 | | fullyLoaded | 4219.100 | 4273.500 | 3760 | 4366 | 168.554 | 4366 | | uss | 21.086 | 21.014 | 20.195 | 22 | 0.654 | 22 | | rss | 43.574 | 43.494 | 42.656 | 44.523 | 0.683 | 44.523 | | pss | 25.279 | 25.204 | 24.378 | 26.206 | 0.665 | 26.206 | wip with all scripts in <head> and deferred, using l20n-service and l20n-client | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | -------- | ------ | ------ | ------- | ------ | | navigationLoaded | 3376.500 | 3424.500 | 2893 | 3530 | 169.302 | 3530 | | navigationInteractive | 3395 | 3444.500 | 2912 | 3546 | 168.894 | 3546 | | visuallyLoaded | 4113.200 | 4134 | 3675 | 4266 | 154.182 | 4266 | | contentInteractive | 4113.700 | 4134.500 | 3675 | 4266 | 154.358 | 4266 | | fullyLoaded | 4114.300 | 4135 | 3676 | 4267 | 154.222 | 4267 | | uss | 22.368 | 22.732 | 21.785 | 22.766 | 0.469 | 22.766 | | rss | 44.818 | 45.184 | 44.230 | 45.219 | 0.470 | 45.219 | | pss | 26.547 | 26.912 | 25.963 | 26.945 | 0.469 | 26.945 | This might get event faster once we break up the l10n resources on the per-view basis (bug 1209256).
Another perf check of my wip after bug 1210691 landed in master. master: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | -------- | ------ | ------ | ------- | ------ | | navigationLoaded | 3372.500 | 3392 | 2881 | 3636 | 191.181 | 3636 | | navigationInteractive | 3395.300 | 3417.500 | 2902 | 3654 | 190.868 | 3654 | | visuallyLoaded | 4024.500 | 4035.500 | 3589 | 4278 | 176.409 | 4278 | | contentInteractive | 4024.800 | 4035.500 | 3589 | 4279 | 176.586 | 4279 | | fullyLoaded | 4098.100 | 4111 | 3673 | 4341 | 172.108 | 4341 | | uss | 20.373 | 20.637 | 19.562 | 21.637 | 0.693 | 21.637 | | pss | 24.492 | 24.750 | 23.670 | 25.767 | 0.703 | 25.767 | | rss | 42.659 | 42.907 | 41.816 | 43.953 | 0.720 | 43.953 | wip: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | -------- | ------ | ------ | ------- | ------ | | navigationLoaded | 3361.400 | 3377 | 2928 | 3522 | 158.114 | 3522 | | navigationInteractive | 3383.300 | 3401.500 | 2947 | 3539 | 158.187 | 3539 | | visuallyLoaded | 3878.700 | 3891.500 | 3486 | 4037 | 149.117 | 4037 | | contentInteractive | 3879.100 | 3892.500 | 3487 | 4038 | 148.978 | 4038 | | fullyLoaded | 3956 | 3968.500 | 3584 | 4101 | 140.155 | 4101 | | uss | 19.791 | 19.629 | 19.539 | 20.176 | 0.249 | 20.176 | | pss | 23.896 | 23.733 | 23.646 | 24.281 | 0.248 | 24.281 | | rss | 42.040 | 41.875 | 41.793 | 42.426 | 0.249 | 42.426 | That's around -150ms on visuallyLoaded and -1mb in all memory measurements.
Every now and then I'm getting an error in my wip: Error: Unable to establish a connection with "l20n". Either the target endpoint is not alive or the Service is not `.listen()`ing. This seems to only happen during the first launch of the app after a reboot and only for index.html. Other views have their l20n clients created and connect to the service properly. index.html is special in that it has both the l20n/service and l20n/client files in use. The former is used to create the service for the whole app while the latter is used to localize music-tab-bar and music-overlays. I tried to narrow this down to a minimal testcase reproduce, but so far in vein. Justin, if the user navigates to multiple views in the app, is there any mechanism to unregister the ones which are not currently visible?
Flags: needinfo?(jdarcangelo)
Thanks, Justin! The error is now gone :) I'm currently working on a way to destroy l10n contexts when views are destroyed: bug 1211561. I also think I'd prefer to first land this and then split the properties files in bug 1209256. Setting dependencies accordingly.
Blocks: 1209256
No longer depends on: 1209256
Assignee: nobody → stas
Status: NEW → ASSIGNED
Comment on attachment 8669318 [details] [review] [gaia] stasm:1210720-music-l20n-bridge > mozilla-b2g:master This is ready for your review, Justin. The PR is actually really simple because all the required changes to l20n.js landed in bug 1212365 Treeherder: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=1e467143a35016fdb62ad6c36d54327df62a6105 Is it enough to listen to the 'disconnect' event in the service to handle the destruction of views? Here's what I'm doing right now: https://github.com/l20n/l20n.js/blob/962b9a4b82b91a25de63472b2fb783d34355707b/src/runtime/bridge/service.js#L19
Attachment #8669318 - Flags: review?(jdarcangelo)
(In reply to Staś Małolepszy :stas from comment #7) > This is ready for your review, Justin. The PR is actually really simple > because all the required changes to l20n.js landed in bug 1212365 Perhaps it's more helpful if I link to the l20n.js bugs: bug 1210719 and bug 1211561.
(In reply to Staś Małolepszy :stas from comment #7) > Comment on attachment 8669318 [details] [review] > [gaia] stasm:1210720-music-l20n-bridge > mozilla-b2g:master > > This is ready for your review, Justin. The PR is actually really simple > because all the required changes to l20n.js landed in bug 1212365 > > Treeherder: > https://treeherder.mozilla.org/#/ > jobs?repo=gaia&revision=1e467143a35016fdb62ad6c36d54327df62a6105 > > Is it enough to listen to the 'disconnect' event in the service to handle > the destruction of views? Here's what I'm doing right now: > > https://github.com/l20n/l20n.js/blob/ > 962b9a4b82b91a25de63472b2fb783d34355707b/src/runtime/bridge/service.js#L19 I believe so, but we better ask :wilsonpage to be safe.
Flags: needinfo?(wilsonpage)
Comment on attachment 8669318 [details] [review] [gaia] stasm:1210720-music-l20n-bridge > mozilla-b2g:master LGTM. Ship it!
Attachment #8669318 - Flags: review?(jdarcangelo) → review+
New results from Flame-kk, runs=31 master: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------ | -------- | | navigationLoaded | 780.355 | 776 | 722 | 843 | 27.168 | 822.900 | | navigationInteractive | 803.935 | 801 | 754 | 862 | 24.732 | 844.800 | | visuallyLoaded | 1378.258 | 1372 | 1315 | 1444 | 26.833 | 1439.550 | | contentInteractive | 1378.387 | 1372 | 1315 | 1444 | 26.753 | 1439.550 | | fullyLoaded | 1513.290 | 1507 | 1448 | 1580 | 27.194 | 1570.750 | | pss | 17.963 | 17.913 | 17.804 | 18.480 | 0.150 | 18.217 | | rss | 33.298 | 33.297 | 33.223 | 33.562 | 0.054 | 33.339 | | uss | 13.830 | 13.824 | 13.750 | 14.109 | 0.057 | 13.870 | patch: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------- | -------- | | navigationLoaded | 787.903 | 784 | 745 | 860 | 24.991 | 836.700 | | navigationInteractive | 810.290 | 808 | 770 | 880 | 23.727 | 858.750 | | visuallyLoaded | 1298.677 | 1298 | 1247 | 1366 | 28.386 | 1358.150 | | contentInteractive | 1299.032 | 1298 | 1247 | 1366 | 28.392 | 1358.200 | | fullyLoaded | 1420.935 | 1391 | 1353 | 2250 | 154.176 | 1462.750 | | uss | 13.971 | 14.039 | 13.719 | 14.145 | 0.131 | 14.121 | | pss | 18.212 | 18.193 | 17.812 | 18.487 | 0.195 | 18.481 | | rss | 33.406 | 33.461 | 33.145 | 33.555 | 0.123 | 33.551 | So, around 100ms perf win is preserved as reported in comment 2, but I the memory win is not there. We actually seem to be regressing memory with this patch by ~150kb. Can anyone try to reproduce it before we land? I'm wondering if it's a fluke in my testing or not.
I also run a music-old in the same setup on todays master: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------ | -------- | | navigationLoaded | 802.774 | 801 | 770 | 895 | 23.474 | 834 | | navigationInteractive | 971.581 | 971 | 939 | 1057 | 24.464 | 1021.350 | | visuallyLoaded | 1087.323 | 1085 | 1011 | 1204 | 50.210 | 1164.700 | | contentInteractive | 1087.710 | 1085 | 1012 | 1205 | 50.487 | 1165.650 | | fullyLoaded | 1826.161 | 1811 | 1714 | 2023 | 63.594 | 1973.800 | | uss | 12.417 | 12.418 | 12.250 | 12.508 | 0.039 | 12.469 | | rss | 32.167 | 32.168 | 32 | 32.250 | 0.039 | 32.218 | | pss | 16.767 | 16.691 | 16.488 | 16.973 | 0.152 | 16.971 | Seems like the visuallyLoaded mark is fired ~200ms later in NGA music, but fulyLoaded 400ms earlier. Navigation is loaded around the same moment and becomes interactive 200ms earlier than it used to. With memory, NGA is around 1.4mb behind. That's much narrower gap than what what Eli reported in the dev.gaia thread, which is a good news. Music NGA is getting close to outperform OGA :)
(previous comment contains wrong data, tagging it as obsolete and reposting with proper numbers) Oh, that's interesting... So, my last comment results were from Flame, but what I forgot to mention is that this Flame had one song on it. That means that its visuallyLoaded and USS might have been affected by DB loading. I decided to test all three scenarios again on Z3 and Flame without any songs to have the "raw" setup. Here are the results: Flame-kk: music-oga: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------ | -------- | | navigationLoaded | 798.129 | 801 | 751 | 841 | 21.498 | 833.700 | | navigationInteractive | 967.839 | 969 | 928 | 1009 | 19.013 | 1001 | | visuallyLoaded | 1041.548 | 1037 | 1003 | 1085 | 20.127 | 1081.800 | | contentInteractive | 1041.677 | 1037 | 1003 | 1085 | 20.204 | 1081.850 | | fullyLoaded | 1708.806 | 1707 | 1565 | 1867 | 54.114 | 1787.750 | | uss | 12.649 | 12.645 | 12.465 | 12.727 | 0.047 | 12.715 | | rss | 32.281 | 32.281 | 32.074 | 32.363 | 0.049 | 32.348 | | pss | 17.071 | 17.089 | 16.597 | 17.197 | 0.124 | 17.176 | music-nga master: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------ | -------- | | navigationLoaded | 780.032 | 778 | 697 | 874 | 38.613 | 840.850 | | navigationInteractive | 803.419 | 802 | 721 | 899 | 36.590 | 861.750 | | visuallyLoaded | 1436.065 | 1438 | 1366 | 1551 | 32.971 | 1487.200 | | contentInteractive | 1436.323 | 1438 | 1368 | 1551 | 32.737 | 1487.200 | | fullyLoaded | 1512.677 | 1519 | 1438 | 1585 | 29.627 | 1559.900 | | uss | 13.247 | 13.254 | 12.984 | 13.320 | 0.053 | 13.285 | | rss | 32.454 | 32.465 | 32.184 | 32.527 | 0.054 | 32.492 | | pss | 17.308 | 17.289 | 16.917 | 17.571 | 0.146 | 17.561 | music-nga patch: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | -------- | ------ | ------ | ------ | ------ | -------- | | navigationLoaded | 790.581 | 792 | 746 | 839 | 22.941 | 835.150 | | navigationInteractive | 812.290 | 812 | 770 | 865 | 22.603 | 855.250 | | visuallyLoaded | 1313.226 | 1306 | 1251 | 1389 | 29.180 | 1363.800 | | contentInteractive | 1313.516 | 1306 | 1251 | 1389 | 29.152 | 1363.800 | | fullyLoaded | 1376.871 | 1375 | 1288 | 1500 | 39.535 | 1439 | | uss | 13.189 | 13.184 | 13.102 | 13.438 | 0.050 | 13.211 | | pss | 17.400 | 17.430 | 17.120 | 17.499 | 0.103 | 17.488 | | rss | 32.375 | 32.367 | 32.301 | 32.621 | 0.049 | 32.395 | Z3: music-oga: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | ------- | ------ | ------ | ------ | ------ | ------- | | navigationLoaded | 357.129 | 355 | 318 | 385 | 15.337 | 382.900 | | navigationInteractive | 447.258 | 449 | 384 | 483 | 23.006 | 476.950 | | visuallyLoaded | 509.355 | 513 | 451 | 560 | 31.074 | 556.950 | | contentInteractive | 509.452 | 513 | 451 | 560 | 30.985 | 556.950 | | fullyLoaded | 748.871 | 752 | 690 | 813 | 29.118 | 797.900 | | rss | 32.403 | 32.414 | 32.316 | 32.461 | 0.042 | 32.461 | | pss | 17.274 | 17.291 | 17.180 | 17.338 | 0.043 | 17.332 | | uss | 12.836 | 12.855 | 12.738 | 12.902 | 0.044 | 12.895 | music-nga master: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | ------- | ------ | ------ | ------ | ------ | ------- | | navigationLoaded | 403.484 | 401 | 361 | 511 | 28.737 | 440.800 | | navigationInteractive | 415.290 | 412 | 372 | 521 | 28.573 | 451.850 | | visuallyLoaded | 769.290 | 760 | 697 | 888 | 43.179 | 880.950 | | contentInteractive | 769.387 | 760 | 698 | 888 | 43.111 | 880.950 | | fullyLoaded | 831.710 | 818 | 747 | 1309 | 97.255 | 923.750 | | rss | 32.473 | 32.473 | 32.285 | 32.547 | 0.046 | 32.535 | | pss | 17.592 | 17.591 | 17.410 | 17.661 | 0.045 | 17.652 | | uss | 13.335 | 13.336 | 13.156 | 13.402 | 0.044 | 13.395 | music-nga patch: | Metric | Mean | Median | Min | Max | StdDev | p95 | | --------------------- | ------- | ------ | ------ | ------ | ------ | ------- | | navigationLoaded | 351.355 | 348 | 319 | 401 | 19.907 | 397.200 | | navigationInteractive | 362.516 | 359 | 331 | 411 | 19.521 | 408.100 | | visuallyLoaded | 657.161 | 653 | 610 | 720 | 28.222 | 711.150 | | contentInteractive | 657.194 | 653 | 610 | 720 | 28.191 | 711.150 | | fullyLoaded | 685.387 | 685 | 629 | 767 | 30.248 | 736.900 | | uss | 13.245 | 13.246 | 13.195 | 13.320 | 0.030 | 13.308 | | rss | 32.371 | 32.371 | 32.320 | 32.438 | 0.029 | 32.433 | | pss | 17.501 | 17.500 | 17.453 | 17.573 | 0.030 | 17.563 |
Unfortunately I'm seeing some Gij failures: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=81918af334ce5cf9d0b83f9f17140914d83b5862 TEST-UNEXPECTED-FAIL | apps/music/test/marionette/playlist_test.js | Music player playlist Default playlists Most played playlist sort order. moztrap:3676,3677 AssertionError: Exception NoSuchElement: NoSuchElement: Unable to locate element: iframe.active[src*="/views/playlists/index.html"] TEST-UNEXPECTED-FAIL | apps/music/test/marionette/playlist_test.js | Music player playlist Default playlists Shuffle all sort order. moztrap:2357 AssertionError: Exception AssertionError: we didn't randomise And I can't get the whole suite to finish.
(In reply to Staś Małolepszy :stas from comment #7) > Is it enough to listen to the 'disconnect' event in the service to handle > the destruction of views? Here's what I'm doing right now: > > https://github.com/l20n/l20n.js/blob/ > 962b9a4b82b91a25de63472b2fb783d34355707b/src/runtime/bridge/service.js#L19 I'm afraid the 'disconnect' event will only be called if a `client.destroy()` or `client.disconnect()` [2] are called manually. I suggest you call `client.disonnect()` on the window 'pagehide' event and `client.connect()` on the window 'pageshow' event. The reason this is not done automatically is because we don't have the same hooks when a client lives inside some form of worker; meaning devs can't reliably depend on it. Worker's do have a 'closed' event, but we are unable to send a postMessage() in the given execution window :( [1] https://github.com/gaia-components/bridge/blob/master/src/client.js#L348 [2] https://github.com/gaia-components/bridge/blob/master/src/client.js#L162
Flags: needinfo?(wilsonpage)
Thanks, Wilson! I'll take a closer look at this after the weekend. I'm also working on some statistical tooling for raptor to be able to better gauge the perf and mem impact of this patch. I'll report back mid-next-week.
Depends on: 1214138
I rebased and got a green build: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=62a6dd4f3ab03b3e90152c35e6270519b600a1dd Landed in https://github.com/mozilla-b2g/gaia/commit/fa891bd32badd0dc04cfff28269dbd6c1038408f.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Ha, so glad to see this, thanks! This has been a team effort and Zibi and I have a few other ideas on how to improve the performance of l10n in Gaia. Stay tuned :) Also, this is in no small part also thanks to Justin's fixing bug 1210691 which set the stage for my patch.
Great news! On the other hand, I can see ~900kb regression in memory - https://raptor.mozilla.org/dashboard/script/memory?var-device=flame-kk&var-memory=512&var-branch=master&var-test=cold-launch&panelId=10&fullscreen 3.4.0 will help to cut part of that (around 250kb), but I'm also investigating bug 1216598 which may help more. :stas, any ideas on why do we pay that much? Is that Bridge? I remember you wanted to test oga l20n.js with just empty Bridge created to see what takes that memory. If I understand correctly, there should be no memory cost of switching to server/client architecture unless Bridge costs and if it does cost that much, we should report it.
There also seem to be a regression in fullyLoaded stdev according to raptor. Nothing major, but we introduced more variance - https://raptor.mozilla.org/dashboard/script/measures?var-device=flame-kk&var-memory=512&var-branch=master&var-test=cold-launch&panelId=10&fullscreen It's a change from around 30ms stdev to 70ms stdev. I'd love to see us trying to cut it down below 50 and I, once again, suspect bridge.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: