Closed Bug 636676 Opened 13 years ago Closed 13 years ago

Utils.queryAsync: reuse mozIStorageStatementCallback object

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: philikon, Assigned: philikon)

References

Details

Attachments

(1 file)

Utils:queryAsync() gets called a lot during history and bookmark sync. Every time we call it we create a bunch of new objects and functions. We should try to reuse those as much as possible.
Assignee: nobody → philipp
Summary: Utils:queryAsync: reuse mozIStorageStatementCallback object → Utils.queryAsync: reuse mozIStorageStatementCallback object
Attached patch v1Splinter Review
Avoid any kind of unnecessary object creation concerning Utils.queryAsync:

* Define most of the mozIStorageCallback outside of queryAsync so that the functions don't have to be recreated on every single invocation.

* Require the names parameter to be an array, if it's provided at all. No more lazy conversion from string to array.

* Lazily create the results array. If no result rows are returned and no column names were given, it's not created and null is returned.

* Globally define column names in bookmarks and history engine.
Attachment #515851 - Flags: review?(rnewman)
Nominating for a Fennec blocker as this gets rid of ca. 15*N object creations (where N is the number of records synced down in the initial sync) -- a huge saving.
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Whiteboard: [has patch][needs review rnewman]
tracking-fennec: ? → 2.0+
Comment on attachment 515851 [details] [diff] [review]
v1

+    this._log.trace("Cols matching GUID " + guid + ": " +

Back to "Rows" plz :D

+      "SELECT guid FROM moz_formhistory WHERE guid = :guid");

Please add " LIMIT 1". Clue for the DB, and safer as we're checking == 1.

+  _checkGUIDPageAnnotationCols: ["place_id", "name_id", "anno_id",
+                                   "anno_date"],

Nit: indentation.

+        this.results.push(item);
+        for each (name in this.names) {
+          item[name] = row.getResultByName(name);
+        }

Nit: please switch these two clauses for clarity.

Otherwise, looks good, and passes tests with my last three changes in place.
Attachment #515851 - Flags: review?(rnewman) → review+
Whiteboard: [has patch][needs review rnewman] → [has patch][has review]
Landed in fx-sync: https://hg.mozilla.org/services/fx-sync/rev/43098e5a23f6
Whiteboard: [has patch][has review] → [fixed in fx-sync]
Merged to s-c: http://hg.mozilla.org/services/services-central/rev/3dd3e050beaa
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in services]
Perhaps not the time to change APIs but maybe it would be good to pass in the names with the statement instead of when querying. It would prevent reusing the same statement (perhaps SELECT *) from multiple query that want different sets of names though.

That might make it saner than having separate arrays for the names stored on some other object. And would maybe make it easier to write wrapper code to memoize statement creation. (Which could be used in those 1-off cases in form data that doesn't yet cache the statement/array.)
(In reply to comment #6)
> Perhaps not the time to change APIs but maybe it would be good to pass in the
> names with the statement instead of when querying. It would prevent reusing the
> same statement (perhaps SELECT *) from multiple query that want different sets
> of names though.

It's also not how storage API is laid out.

> That might make it saner than having separate arrays for the names stored on
> some other object. And would maybe make it easier to write wrapper code to
> memoize statement creation. (Which could be used in those 1-off cases in form
> data that doesn't yet cache the statement/array.)

Maybe. I agree it would be nicer and saner. Possibly something we can do when we go properly async (as opposed to "here I'll just spin the event loop while we're waiting" pretend-async).
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/3dd3e050beaa
Whiteboard: [fixed in fx-sync][fixed in services]
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: