Closed
Bug 722242
(placesIdleContention)
Opened 12 years ago
Closed 12 years ago
Avoid thread contention on idle maintenance
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: mak, Assigned: Paolo)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Snappy])
Attachments
(1 file, 1 obsolete file)
11.14 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
Taras got twice contention on idle maintenance, after resuming his laptop from sleep, this is caused by the synchronous part of telemetry gathering (page_size query) in PlacesDBUtils, that part should be made async, asap. I'll figure out the change with Paolo and try to bring this fix out along the week.
Reporter | ||
Updated•12 years ago
|
Whiteboard: [Snappy]
Reporter | ||
Comment 1•12 years ago
|
||
the related code is here (PLACES_DATABASE_SIZE_PER_PAGE_B) http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/PlacesDBUtils.jsm#923 right now that code allows 2 kind of "measures" a function returning synchronously or a statement executed asynchronously, the above measure can't be any of those though, since it's made up of 3 separate queries. An interesting fact is that if we asynchronously query for "PRAGMA page_count", we already queried before for the other 2 values that we may cache. So, what we could do is to cache all values we get, and likely manage each telemetry entry as a {statement+callback}, the ones without a statement will just run the callback. Or we may evaluate other ideas.
Comment 2•12 years ago
|
||
We should also uplift this fix to whatever stable releases we can.
Assignee | ||
Comment 3•12 years ago
|
||
I've verified that with this patch applied, test_telemetry.js still passes.
Attachment #593078 -
Flags: review?(mak77)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 593078 [details] [diff] [review] The patch Review of attachment 593078 [details] [diff] [review]: ----------------------------------------------------------------- It looks good, just a couple minor cleanups left. ::: toolkit/components/places/PlacesDBUtils.jsm @@ +954,5 @@ > + for (let i = 0; i < probes.length; i++) { > + let probe = probes[i]; > + let histogram = Services.telemetry.getHistogramById(probe.histogram); > + > + let reportTelemetry = function PDBUT_reportTelemetry(aResultValue) { please just define as function reportTelemetry(... while this absolutely correct, it makes easier to see this is just an internal helper. Also would be nice to move this out of the loop so we save some GC stuff. the function would get (aProbe, aValue) and get the histogram directly from the probe, since it's the only remaining point needing the histogram. @@ +957,5 @@ > + > + let reportTelemetry = function PDBUT_reportTelemetry(aResultValue) { > + try { > + if ("callback" in probe) { > + aResultValue = probe.callback(aResultValue); this may look confusing, let's define a local var initialized to aResultValue, and overwrite its value here. then use the var value.
Attachment #593078 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #593078 -
Attachment is obsolete: true
Attachment #593130 -
Flags: review?(mak77)
Reporter | ||
Comment 6•12 years ago
|
||
Comment on attachment 593130 [details] [diff] [review] Updated patch Review of attachment 593130 [details] [diff] [review]: ----------------------------------------------------------------- Thank you!
Attachment #593130 -
Flags: review?(mak77) → review+
Reporter | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa19ec8815
Target Milestone: --- → mozilla13
Updated•12 years ago
|
Blocks: StorageJank
Updated•12 years ago
|
Alias: placesIdleContention
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2aaa19ec8815
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•