Closed Bug 1109704 Opened 10 years ago Closed 10 years ago

Expose detailed Settings locks usage in about:memory

Categories

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

33 Branch
ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: gerard-majax, Assigned: gerard-majax)

References

Details

Attachments

(2 files, 4 obsolete files)

To help investigate bug 1095034, let's get precise figures of the use of get/set on locks.
Attached file Example (deleted) —
Example after just a boot of B2G
We want to have a better breakdown of how many get/set operations we are doing for each setting lock.
Attachment #8534407 - Flags: review?(kyle)
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.
Attachment #8534407 - Flags: review?(kyle) → review-
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
Flags: needinfo?(kyle)
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.
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.
Attachment #8534407 - Attachment is obsolete: true
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.
Attachment #8534881 - Flags: review?(kyle)
Depends on: 1110091
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?
Attachment #8534881 - Flags: review?(kyle) → review-
Flags: needinfo?(kyle)
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.
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.
Attachment #8534881 - Attachment is obsolete: true
Rebased on top of bug 1110091.
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.
Attachment #8536007 - Attachment is obsolete: true
I'll also add some reporting from SettingsService component.
(In reply to Alexandre LISSY :gerard-majax from comment #13) > I'll also add some reporting from SettingsService component. Or not.
Attachment #8537776 - Flags: review?(kyle)
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.
Flags: needinfo?(kyle)
Sounds fine to me.
Flags: needinfo?(kyle)
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.
Attachment #8537776 - Attachment is obsolete: true
Attachment #8537776 - Flags: review?(kyle)
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 :)
Attachment #8537957 - Flags: review?(kyle)
Comment on attachment 8537957 [details] [diff] [review] Expose details settings informations in about:memory r=qdot Review of attachment 8537957 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8537957 - Flags: review?(kyle) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: