Last Comment Bug 722242 - (placesIdleContention) Avoid thread contention on idle maintenance
: Avoid thread contention on idle maintenance
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla13
Assigned To: :Paolo Amadini
: Marco Bonardo [::mak]
Depends on:
Blocks: StorageJank 722188 722212
  Show dependency treegraph
Reported: 2012-01-30 01:21 PST by Marco Bonardo [::mak]
Modified: 2012-09-11 04:01 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

The patch (11.17 KB, patch)
2012-01-31 06:41 PST, :Paolo Amadini
mak77: feedback+
Details | Diff | Splinter Review
Updated patch (11.14 KB, patch)
2012-01-31 09:21 PST, :Paolo Amadini
mak77: review+
Details | Diff | Splinter Review

Description User image Marco Bonardo [::mak] 2012-01-30 01:21:43 PST
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 User image Marco Bonardo [::mak] 2012-01-30 01:45:09 PST
the related code is here (PLACES_DATABASE_SIZE_PER_PAGE_B)

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 User image (dormant account) 2012-01-30 02:26:26 PST
We should also uplift this fix to whatever stable releases we can.
Comment 3 User image :Paolo Amadini 2012-01-31 06:41:01 PST
Created attachment 593078 [details] [diff] [review]
The patch

I've verified that with this patch applied, test_telemetry.js still passes.
Comment 4 User image Marco Bonardo [::mak] 2012-01-31 08:35:22 PST
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 User image :Paolo Amadini 2012-01-31 09:21:39 PST
Created attachment 593130 [details] [diff] [review]
Updated patch
Comment 6 User image Marco Bonardo [::mak] 2012-02-01 05:26:13 PST
Comment on attachment 593130 [details] [diff] [review]
Updated patch

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

Thank you!
Comment 8 User image Ed Morley [:emorley] 2012-02-03 11:43:15 PST

Note You need to log in before you can comment on or make changes to this bug.