1109704 - Expose detailed Settings locks usage in about:memory

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:gerard-majax]

To help investigate bug 1095034, let's get precise figures of the use of get/set on locks.

Comment 1

[:gerard-majax]

Attachment: Example

Example after just a boot of B2G

Comment 2

[:gerard-majax]

Attachment: Expose details settings tasks informations r=qdot

We want to have a better breakdown of how many get/set operations we are
doing for each setting lock.

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8534407 [details] [diff] [review]
Expose details settings tasks informations r=qdot

Review of attachment 8534407 [details] [diff] [review]:
-----------------------------------------------------------------

It doesn't look like this ever cleans up, i.e. if a lock dies, we still keep the id in the checker. We create MANY (I would figure thousands?) of locks during the scope of the phone lifetime, so it seems like we might want to just keep these records when debugging? Feel free to re-r? me if you disagree, I'm just making sure this is the intent and we aren't going to slow things down by doing this in all cases.

Comment 4

[:gerard-majax]

Yes, that was on purpose, I should have made this more clear. I share your concern, though, but except hiding behind a pref or only in debug/verbose mode, I don't see a better way.

Given that we turn on debug by default to expose errors, either we hide it under a new pref or we just make it part of the verbose mode.

I would be in favor of

Comment 5

[:gerard-majax]

Yes, that was on purpose, I should have made this more clear. I share your concern, though, but except hiding behind a pref or only in debug/verbose mode, I don't see a better way.

Given that we turn on debug by default to expose errors, either we hide it under a new pref or we just make it part of the verbose mode.

I would be in favor of a new pref, because we may want to track this usage without the burden of spamming the logcat with verbose mode.

Comment 6

[:gerard-majax]

Attachment: Expose details settings tasks informations r=qdot

We want to have a better breakdown of how many get/set operations we are
doing for each setting lock. Given that we keep track of all locks, this
may consume quite some memory, so we introduce a new preference to
enable this feature.

Comment 7

[:gerard-majax]

Comment on attachment 8534881 [details] [diff] [review]
Expose details settings tasks informations r=qdot

Added the dom.mozSettings.trackTasksUsage pref to enable the feature and avoid consuming too much memory in the general usecase.

Comment 8

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8534881 [details] [diff] [review]
Expose details settings tasks informations r=qdot

Review of attachment 8534881 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/settings/SettingsRequestManager.jsm
@@ +747,5 @@
>    },
>  
>    collectReports: function(aCallback, aData, aAnonymize) {
> +    // if TRACK is not enabled, then, nothing to give back
> +    if (!TRACK) {

This isn't necessarily true. We can still check number of tasks per locks alive even if track isn't on, which is a good metric to know if we need to turn it on. You could move this check to continue through the for loop after the "Alive tasks for this lock", and then return early after the for loop?

Comment 9

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Should say, I do like the perf idea. Just wondering about if we want to have some reporting that just uses things we store already when it's off.

Comment 10

[:gerard-majax]

Attachment: Expose details settings tasks informations r=qdot

We want to have a better breakdown of how many get/set operations we are
doing for each setting lock. Given that we keep track of all locks, this
may consume quite some memory, so we introduce a new preference to
enable this feature.

Comment 11

[:gerard-majax]

Rebased on top of bug 1110091.

Comment 12

[:gerard-majax]

Attachment: Expose details settings tasks informations r=qdot

We want to have a better breakdown of how many get/set operations we are
doing for each setting lock. Given that we keep track of all locks, this
may consume quite some memory, so we introduce a new preference to
enable this feature.

Comment 13

[:gerard-majax]

I'll also add some reporting from SettingsService component.

Comment 14

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #13)
> I'll also add some reporting from SettingsService component.

Or not.

Comment 15

[:gerard-majax]

Ok, Kyle, I need your opinion, but after giving it a second tought, here is mine: we currently have no lock tracking nor memory reporting for SettingsService, like we have on SettingsManager and SettingsRequestManager.

I think that it would be valuable that we get some, otherwise we may miss other abuses of the Settings API/service (like the GPS one, which would have been much easier to spot). So, while I'm exposing more informations about locks, I think it makes sense to also cover the SettingsService case.

Comment 16

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Sounds fine to me.

Comment 17

[:gerard-majax]

Attachment: Expose details settings informations in about:memory r=qdot

We want to have a better breakdown of how many get/set operations we are
doing for each setting lock. Given that we keep track of all locks, this
may consume quite some memory, so we introduce a new preference to
enable this feature. We also add memory reporting for the SettingService
lock.

Comment 18

[:gerard-majax]

Comment on attachment 8537957 [details] [diff] [review]
Expose details settings informations in about:memory r=qdot

With stuff on SettingsService. I have not tested on device, but hacked in Mulet and it did the trick :)

Comment 19

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8537957 [details] [diff] [review]
Expose details settings informations in about:memory r=qdot

Review of attachment 8537957 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 20

[:gerard-majax]

https://tbpl.mozilla.org/?tree=Try&rev=bec9adaa805a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/013212b16059

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/013212b16059