Bug 722242 (placesIdleContention)

Avoid thread contention on idle maintenance

RESOLVED FIXED in mozilla13

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mak, Assigned: Paolo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy])

Attachments

(1 attachment, 1 obsolete attachment)

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

Comment 2

6 years ago
We should also uplift this fix to whatever stable releases we can.
(Assignee)

Comment 3

6 years ago
Created attachment 593078 [details] [diff] [review]
The patch

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+
(Assignee)

Comment 5

6 years ago
Created attachment 593130 [details] [diff] [review]
Updated patch
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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa19ec8815
Target Milestone: --- → mozilla13
Blocks: 699820
Alias: placesIdleContention

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/2aaa19ec8815
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.