Destroy contexts when view is disposed of

RESOLVED FIXED

Status

Firefox OS
Gaia::L10n
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: stas, Assigned: stas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

39 bytes, text/x-github-pull-request
gandalf
: review+
Details | Review | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1210719
(Assignee)

Comment 1

2 years ago
Created attachment 8670756 [details] [review]
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.
Assignee: nobody → stas
Status: NEW → ASSIGNED
Attachment #8670756 - Flags: review?(gandalf)
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
PR updated using the cache.get(resid + code + src) approach.
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.
Attachment #8670756 - Flags: review?(gandalf) → review+
(Assignee)

Comment 6

2 years ago
https://github.com/l20n/l20n.js/commit/845ee6095d7912cbf82fb10ad89a3fefbe65df5d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.