1211561 - Destroy contexts when view is disposed of

Status: RESOLVED FIXED

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

Description

[Staś Małolepszy :stas]

In NGA, unused views may be disposed of.  We should provided a way to destroy the corresponding contexts and delete the cached resources if they're not need for other contexts to work properly.

Comment 1

[Staś Małolepszy :stas]

Attachment: Pull request

This is ready for your review, Zibi.  Instead of a cache object in Env and an array of resIds in each Context, I implemented two Maps in Env.  And instead of keeping track of which resources are needed by which context I just verify this manually in destroyContext by iterating over all non-destroyed contexts.

Comment 2

[Staś Małolepszy :stas]

raptor results for fm:

master:

| Metric                | Mean     | Median   | Min    | Max    | StdDev  | p95    |
| --------------------- | -------- | -------- | ------ | ------ | ------- | ------ |
| navigationLoaded      | 3252     | 3281.500 | 2747   | 3483   | 181.615 | 3483   |
| navigationInteractive | 3513.400 | 3567.500 | 2919   | 3628   | 200.068 | 3628   |
| visuallyLoaded        | 3513.700 | 3567.500 | 2919   | 3628   | 200.157 | 3628   |
| contentInteractive    | 3514.300 | 3568     | 2920   | 3629   | 200.077 | 3629   |
| fullyLoaded           | 3696     | 3742.500 | 3221   | 3815   | 161.480 | 3815   |
| uss                   | 16.180   | 16.135   | 16.094 | 16.645 | 0.156   | 16.645 |
| pss                   | 20.033   | 19.992   | 19.941 | 20.483 | 0.151   | 20.483 |
| rss                   | 37.765   | 37.738   | 37.645 | 38.191 | 0.146   | 38.191 |


patch:

| Metric                | Mean     | Median   | Min    | Max    | StdDev | p95    |
| --------------------- | -------- | -------- | ------ | ------ | ------ | ------ |
| navigationLoaded      | 3274.600 | 3293.500 | 3082   | 3366   | 70.007 | 3366   |
| navigationInteractive | 3583.200 | 3606     | 3314   | 3650   | 93.445 | 3650   |
| visuallyLoaded        | 3583.400 | 3606     | 3314   | 3650   | 93.466 | 3650   |
| contentInteractive    | 3583.900 | 3606.500 | 3317   | 3650   | 92.607 | 3650   |
| fullyLoaded           | 3752.700 | 3773.500 | 3512   | 3821   | 83.288 | 3821   |
| uss                   | 16.196   | 16.148   | 16.109 | 16.617 | 0.143  | 16.617 |
| pss                   | 20.061   | 20.017   | 19.979 | 20.464 | 0.137  | 20.464 |
| rss                   | 37.795   | 37.754   | 37.719 | 38.168 | 0.128  | 38.168 |


I'll try with cache being an object not a map and report back.

Comment 3

[Staś Małolepszy :stas]

This might be an exercise in futility because the numbers are not stable enough.  Still, I ran more tests:


cache.get(resid)[code + src]

| Metric                | Mean     | Median   | Min    | Max    | StdDev | p95    |
| --------------------- | -------- | -------- | ------ | ------ | ------ | ------ |
| navigationLoaded      | 3274.600 | 3293.500 | 3082   | 3366   | 70.007 | 3366   |
| navigationInteractive | 3583.200 | 3606     | 3314   | 3650   | 93.445 | 3650   |
| visuallyLoaded        | 3583.400 | 3606     | 3314   | 3650   | 93.466 | 3650   |
| contentInteractive    | 3583.900 | 3606.500 | 3317   | 3650   | 92.607 | 3650   |
| fullyLoaded           | 3752.700 | 3773.500 | 3512   | 3821   | 83.288 | 3821   |
| uss                   | 16.196   | 16.148   | 16.109 | 16.617 | 0.143  | 16.617 |
| pss                   | 20.061   | 20.017   | 19.979 | 20.464 | 0.137  | 20.464 |
| rss                   | 37.795   | 37.754   | 37.719 | 38.168 | 0.128  | 38.168 |


cache[resid][code + src]

| Metric                | Mean     | Median   | Min    | Max    | StdDev  | p95    |
| --------------------- | -------- | -------- | ------ | ------ | ------- | ------ |
| navigationLoaded      | 3241.900 | 3275.500 | 2733   | 3421   | 180.650 | 3421   |
| navigationInteractive | 3552.600 | 3575     | 3242   | 3746   | 122.812 | 3746   |
| visuallyLoaded        | 3552.800 | 3575.500 | 3242   | 3746   | 122.877 | 3746   |
| contentInteractive    | 3553.200 | 3576     | 3243   | 3746   | 122.702 | 3746   |
| fullyLoaded           | 3729     | 3746     | 3453   | 3920   | 111.737 | 3920   |
| uss                   | 16.184   | 16.135   | 16.059 | 16.711 | 0.178   | 16.711 |
| pss                   | 20.042   | 19.997   | 19.900 | 20.574 | 0.180   | 20.574 |
| rss                   | 37.779   | 37.742   | 37.609 | 38.320 | 0.186   | 38.320 |


cache.get(resid + code + src)

| Metric                | Mean     | Median   | Min    | Max    | StdDev  | p95    |
| --------------------- | -------- | -------- | ------ | ------ | ------- | ------ |
| navigationLoaded      | 3254     | 3300     | 2749   | 3361   | 170.683 | 3361   |
| navigationInteractive | 3561.200 | 3601     | 3203   | 3679   | 129.837 | 3679   |
| visuallyLoaded        | 3561.400 | 3602     | 3203   | 3679   | 129.898 | 3679   |
| contentInteractive    | 3561.800 | 3603     | 3204   | 3679   | 129.753 | 3679   |
| fullyLoaded           | 3736     | 3768.500 | 3396   | 3842   | 125.526 | 3842   |
| uss                   | 16.193   | 16.139   | 16.113 | 16.695 | 0.168   | 16.695 |
| rss                   | 37.763   | 37.713   | 37.664 | 38.270 | 0.170   | 38.270 |
| pss                   | 20.050   | 19.995   | 19.974 | 20.555 | 0.169   | 20.555 |


I think I like the last option the most.  It's flat so it should use less memory, but still uses a Map which is the more appropriate choice here.

Comment 4

[Staś Małolepszy :stas]

PR updated using the cache.get(resid + code + src) approach.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8670756 [details] [review]
Pull request

Overall, r+.

In the future we may want to pay with memory for performance here and add another Map that has keys for resources and context set for values to not have to loop over all contexts to check if a resource is still used, but I would wait with that until we have more data about number of contexts per Env etc.

Comment 6

[Staś Małolepszy :stas]

https://github.com/l20n/l20n.js/commit/845ee6095d7912cbf82fb10ad89a3fefbe65df5d