Status

defect
P1
normal
RESOLVED FIXED
6 years ago
9 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
Firefox 22

Firefox Tracking Flags

(firefox20 unaffected, firefox21+ fixed, firefox22 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

6 years ago
There are a number of places in FHR where we should be performing transactions instead of a series of individual writes. IIRC SQLite treats "naked" INSERT or UPDATES as implicit transactions. Thus, with synchronous=FULL (the default), any INSERT or UPDATE results in an fsync. Transactions defer the commit to the end, reducing the number of I/O writes and fsyncs and thus making things run faster.

Patch coming shortly.
Assignee

Comment 1

6 years ago
Posted patch Use more transactions, v1 (obsolete) — Splinter Review
Aside from the risks of "errors during collection now result in no data being collected instead of partial data," the only weird thing this does AFAIK is that during registerFields() on subsequent runs, we will get a bunch of empty transactions (transactions being committed with no statements running). I *hope* SQLite optimizes this as a no-op. If not, changing the code to intelligently perform a transaction only if needed will be a little invasive.
Attachment #729254 - Flags: review?(rnewman)
Comment on attachment 729254 [details] [diff] [review]
Use more transactions, v1

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

As far as I can tell from reading the code, if your transaction is DEFERRED, all BEGIN TRANSACTION does is turn off auto-commit. Each statement execution thereafter will begin a transaction if necessary. Commit turns auto-commit back on.

(Measuring needed to see what the real-world cost is.)

If you can trivially avoid executing empty transactions -- at least in DEFERRED mode, which already has delay semantics -- I'd prefer it: we'll avoid the auth checks, the statement parsing, the round trip to the DB, yadda yadda.

Otherwise, this looks good, so r+, but I'd be happy to do a quick re-review of delaying.

Fascinated to see how this affects telemetry…
Attachment #729254 - Flags: review?(rnewman) → review+
(In reply to Gregory Szorc [:gps] from comment #0)
> IIRC SQLite treats
> "naked" INSERT or UPDATES as implicit transactions.

Yes, this is what happens, it's the Durability requirement.

(In reply to Gregory Szorc [:gps] from comment #1)
I *hope* SQLite optimizes this as a no-op. If not, changing the
> code to intelligently perform a transaction only if needed will be a little
> invasive.

I didn't verify, but I think there is no optimization, you may have valid reasons to create empty transactions, like creating an empty EXCLUSIVE transaction to cause the database to lock out any other query for a while (we do this in tests).
Since there's some use case, I think it would be wrong to optimize empty transactions.
Assignee

Comment 4

6 years ago
Eliminated a few empty transactions.

I'm not sure this actually does anything. According to the SQLite manual, deferred transactions don't obtain a lock until a statement is issued. Since no locks are obtained and no data is written, you'd think that the commit would do nothing. Who knows.
Attachment #729254 - Attachment is obsolete: true
Attachment #729652 - Flags: review?(rnewman)
fwiw, easy to check, make a loop of 1000 empty transactions and check IO!
See comment 2! Indeed, deferred transactions don't immediately take a lock. That doesn't mean they're free, so if it's work to avoid empty transactions, you should figure out what they cost. 

Note that you should only make DEFERRED lazy. (Haven't looked at patch; on phone.)
Assignee

Comment 7

6 years ago
We could certainly optimize out empty deferred transactions in Sqlite.jsm! Since BEGIN DEFERRED has no side-effects, this should be harmless. I'll file a bug...
Assignee

Comment 8

6 years ago
I filed bug 854973. While I could patch that real quick, there would be uplift considerations that I want to avoid. We can unhack this patch on m-c when that bug lands.
Assignee

Comment 9

6 years ago
And there's a patch up on bug 854973. Depending on when that lands, we may consider landing v1 on m-c and v2 on aurora. Or, we could land v2 everywhere and unhack 22+ later.
Comment on attachment 729652 [details] [diff] [review]
Use more transactions, v2

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

r+ -- this is better than we currently have -- but we can do better in a follow-up.

::: services/metrics/dataprovider.jsm
@@ +202,5 @@
> +    return this.storage.enqueueTransaction(function registerFields() {
> +      for (let [name, type] of missing) {
> +        this._log.debug("Registering field: " + name + " " + type);
> +        let id = yield this.storage.registerField(this.id, name, type);
> +        this._fields[name] = [id, type];

Again, fine for now, but you should do one or both of two things:

* Define storage.registerFields(missing), which can take care of the transaction for you.
* Do this in a single query, perhaps. I'm not sure if we can get the ID out this way, but if you can, that would be best.

::: services/metrics/storage.jsm
@@ +1150,5 @@
> +          for (let type of missingTypes) {
> +            let params = {name: type};
> +            yield self._connection.executeCached(SQL.addType, params);
> +            let rows = yield self._connection.executeCached(SQL.getTypeID, params);
> +            let id = rows[0].getResultByIndex(0);

This is all fine for now, but I would prefer you rewrite this whole thing to do a single query, rather than iterating (even in a transaction).
Attachment #729652 - Flags: review?(rnewman) → review+
Assignee

Comment 11

6 years ago
(In reply to Richard Newman [:rnewman] from comment #10)
> ::: services/metrics/storage.jsm
> @@ +1150,5 @@
> > +          for (let type of missingTypes) {
> > +            let params = {name: type};
> > +            yield self._connection.executeCached(SQL.addType, params);
> > +            let rows = yield self._connection.executeCached(SQL.getTypeID, params);
> > +            let id = rows[0].getResultByIndex(0);
> 
> This is all fine for now, but I would prefer you rewrite this whole thing to
> do a single query, rather than iterating (even in a transaction).

See bug 833965.
Assignee

Comment 13

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9f88a271a336
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed in services]
Target Milestone: --- → mozilla22
Assignee

Comment 14

6 years ago
We should consider uplifting to 21 because of performance wins.
Assignee

Comment 15

6 years ago
Comment on attachment 729652 [details] [diff] [review]
Use more transactions, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): FHR
User impact if declined: Some FHR database inserts will be much slower than they could be. This is because of excessive fsync(), which may cause a user's computer to run slower.
Testing completed (on m-c, etc.): It's been on m-c for a day and we didn't see an uptick in errors reported by FHR.
Risk to taking this patch (and alternatives if risky): Should be low.
String or IDL/UUID changes made by this patch: None
Attachment #729652 - Flags: approval-mozilla-aurora?
(In reply to Gregory Szorc [:gps] from comment #15)
> Comment on attachment 729652 [details] [diff] [review]
> Use more transactions, v2
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): FHR
> User impact if declined: Some FHR database inserts will be much slower than
> they could be. This is because of excessive fsync(), which may cause a
> user's computer to run slower.
> Testing completed (on m-c, etc.): It's been on m-c for a day and we didn't
> see an uptick in errors reported by FHR.
> Risk to taking this patch (and alternatives if risky): Should be low.
> String or IDL/UUID changes made by this patch: None

Do we have any metrics around the performance wins before/after the patch landed ? I am happy to approve the patch as its low risk but supporting numbers will be even better :)
Assignee

Comment 17

6 years ago
I don't think we isolated the changes from this and bug 848136. We do know that combined they decreased wall time execution for our longest xpcshell from 344s to 128s. We know the changes in this bug decreased the number of implicit commits, which decreased the number of disk writes. We know this will have a perf impact, but we don't have numbers isolating just this patch since they landed at the same time.
Attachment #729652 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla22 → Firefox 22

Updated

9 months ago
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.