Closed Bug 722242 (placesIdleContention) Opened 9 years ago Closed 9 years ago

Avoid thread contention on idle maintenance

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: mak, Assigned: Paolo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Snappy])

Attachments

(1 file, 1 obsolete file)

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.
Whiteboard: [Snappy]
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.
Blocks: 722188
Blocks: 722212
We should also uplift this fix to whatever stable releases we can.
Attached patch The patch (obsolete) — Splinter Review
I've verified that with this patch applied, test_telemetry.js still passes.
Attachment #593078 - Flags: review?(mak77)
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+
Attached patch Updated patchSplinter Review
Attachment #593078 - Attachment is obsolete: true
Attachment #593130 - Flags: review?(mak77)
Comment on attachment 593130 [details] [diff] [review]
Updated patch

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

Thank you!
Attachment #593130 - Flags: review?(mak77) → review+
Alias: placesIdleContention
https://hg.mozilla.org/mozilla-central/rev/2aaa19ec8815
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.