722242 - (placesIdleContention) Avoid thread contention on idle maintenance

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

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.

Comment 1

[Marco Bonardo [:mak]]

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

[(dormant account)]

We should also uplift this fix to whatever stable releases we can.

Comment 3

[:Paolo Amadini]

Attachment: The patch

I've verified that with this patch applied, test_telemetry.js still passes.

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[:Paolo Amadini]

Attachment: Updated patch

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 593130 [details] [diff] [review]
Updated patch

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

Thank you!

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa19ec8815

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2aaa19ec8815