Closed
Bug 636676
Opened 13 years ago
Closed 13 years ago
Utils.queryAsync: reuse mozIStorageStatementCallback object
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0+ | --- |
People
(Reporter: philikon, Assigned: philikon)
References
Details
Attachments
(1 file)
19.49 KB,
patch
|
rnewman
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•13 years ago
|
Assignee: nobody → philipp
Summary: Utils:queryAsync: reuse mozIStorageStatementCallback object → Utils.queryAsync: reuse mozIStorageStatementCallback object
Assignee | ||
Comment 1•13 years ago
|
||
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)
Assignee | ||
Comment 2•13 years ago
|
||
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]
Updated•13 years ago
|
tracking-fennec: ? → 2.0+
Comment 3•13 years ago
|
||
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+
Updated•13 years ago
|
Whiteboard: [has patch][needs review rnewman] → [has patch][has review]
Assignee | ||
Comment 4•13 years ago
|
||
Landed in fx-sync: https://hg.mozilla.org/services/fx-sync/rev/43098e5a23f6
Whiteboard: [has patch][has review] → [fixed in fx-sync]
Assignee | ||
Comment 5•13 years ago
|
||
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]
Comment 6•13 years ago
|
||
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.)
Assignee | ||
Comment 7•13 years ago
|
||
(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).
Assignee | ||
Comment 8•13 years ago
|
||
Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/3dd3e050beaa
Whiteboard: [fixed in fx-sync][fixed in services]
Assignee | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
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.
Description
•