Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10790041:
Add IDB perf tests for random read, with and without an index. (Closed)
|
Created:
8 years, 5 months ago by ericu Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd IDB perf tests for random read, with and without an index.
BUG=137764
TEST=self
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148630
Patch Set 1 #Patch Set 2 : Fix argument order to put(). #Patch Set 3 : Fix index creation and db deletion [but comment out deletion]. #
Total comments: 21
Patch Set 4 : Rolled in code review feedback #Messages
Total messages: 8 (0 generated)
Updated with some fixes for bugs I found in later work. I've commented out db deletion, as it turns out to be horrendously slow--I see 300ms, 45s, and so-long-the-perf-test-times-out, depending perhaps on the properties of the individual database. I'll make a small test case and log a separate bug. +CC the other IDB folks; I see I forgot to CC them on this earlier.
On 2012/07/25 20:58:41, ericu wrote: > I've commented out db deletion, as it turns out to be horrendously slow--I see > 300ms, 45s, and so-long-the-perf-test-times-out, depending perhaps on the > properties of the individual database. I'll make a small test case and log a > separate bug. FWIW, http://code.google.com/p/chromium/issues/detail?id=113179 discusses the slow deletion issue a bit.
lgtm with nits and commentary http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... File chrome/test/data/indexeddb/perf_shared.js (right): http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_shared.js:53: var db; db is global here, but passed as arguments elsewhere. Consistency? http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_shared.js:61: function createDatabase(name, objectStores, handler, errorHandler, options) { So, rather than special casing options for indexes, alec suggested (elsewhere) that you could specify the whole test schema in a single JS object: {name, objectStores: opt_array[{ name, autoIncrement:opt_bool, keyPath:opt_string_or_stringarray, indexes: opt_array[{ name, keyPath:string_or_stringarray, unique:opt_bool, multiEntry:opt_bool }] }] } That doesn't help with the 'same index on every store' directly, though, if that does prove common. http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_shared.js:70: assert(options.indexKeyPath || options.indexKeyPath == ""); I'd go with: assert('indexKeyPath' in options); http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_shared.js:91: } Missing semicolon. (Sorry about the style nits on unchanged code.) http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_shared.js:97: function(e) { Put on previous line to reduce indentation on following lines. http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_shared.js:103: } Missing semicolon. http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... File chrome/test/data/indexeddb/perf_test.js (right): http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_test.js:21: var cleanUpFunc = getCleanUpFunc(testName, Date.now(), onTestComplete); Having the test start time recorded in the call to getCleanUpFunc is a little strange. http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_test.js:48: var objectStoreNames = [storeName]; FYI, a single string is acceptable as well as an array of strings. http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_test.js:117: createDatabase(testName, objectStoreNames, onCreated, onError, options); Might be just a personal thing, but I find tests more readable if the code order follows the execution order, taking advantage of variable and function hoisting (i.e. you can define a function after it is called). So e.g. move this createDatabase() call towards the top, with its callbacks defined below it. Similarly, the callback defined in the getTransaction() call which starts the next batch or calls cleanup could be a named function defined at the end of runOneBatch so that "cleanUpFunc" is basically the last meaningful line in testRandomReads(). http://codereview.chromium.org/10790041/diff/5001/chrome/test/data/indexeddb/... chrome/test/data/indexeddb/perf_test.js:125: [testRandomReads, 1000, 5, 50, false], For readability, maybe define a "const" (var kUseIndex = true;)
https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... File chrome/test/data/indexeddb/perf_shared.js (right): https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_shared.js:53: var db; On 2012/07/26 18:23:30, jsbell wrote: > db is global here, but passed as arguments elsewhere. Consistency? Done. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_shared.js:61: function createDatabase(name, objectStores, handler, errorHandler, options) { On 2012/07/26 18:23:30, jsbell wrote: > So, rather than special casing options for indexes, alec suggested (elsewhere) > that you could specify the whole test schema in a single JS object: > > {name, > objectStores: opt_array[{ > name, > autoIncrement:opt_bool, > keyPath:opt_string_or_stringarray, > indexes: opt_array[{ > name, > keyPath:string_or_stringarray, > unique:opt_bool, > multiEntry:opt_bool > }] > }] > } > > That doesn't help with the 'same index on every store' directly, though, if that > does prove common. Let me think about that one for a later CL. Depending on how complicated the differences between the tests are, that might up with more or less total code/mess. It'll be easier to see which it'll be once I've got more tests in. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_shared.js:70: assert(options.indexKeyPath || options.indexKeyPath == ""); On 2012/07/26 18:23:30, jsbell wrote: > I'd go with: assert('indexKeyPath' in options); Much better, thanks. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_shared.js:91: } On 2012/07/26 18:23:30, jsbell wrote: > Missing semicolon. (Sorry about the style nits on unchanged code.) No prob; it still needs fixing. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_shared.js:97: function(e) { On 2012/07/26 18:23:30, jsbell wrote: > Put on previous line to reduce indentation on following lines. Had to go and reread the style guide on this one; it's been a while. Done. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_shared.js:103: } On 2012/07/26 18:23:30, jsbell wrote: > Missing semicolon. Done. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... File chrome/test/data/indexeddb/perf_test.js (right): https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_test.js:21: var cleanUpFunc = getCleanUpFunc(testName, Date.now(), onTestComplete); On 2012/07/26 18:23:30, jsbell wrote: > Having the test start time recorded in the call to getCleanUpFunc is a little > strange. How about if I call it "getCompletionFunc" instead? It does all the completion stuff, both cleanup and results-publishing. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_test.js:48: var objectStoreNames = [storeName]; On 2012/07/26 18:23:30, jsbell wrote: > FYI, a single string is acceptable as well as an array of strings. I make it a consistent array everywhere so as to keep the type passed to createDatabase consistent. I could check its type inside, but that seemed messier. https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_test.js:117: createDatabase(testName, objectStoreNames, onCreated, onError, options); On 2012/07/26 18:23:30, jsbell wrote: > Might be just a personal thing, but I find tests more readable if the code order > follows the execution order, taking advantage of variable and function hoisting > (i.e. you can define a function after it is called). So e.g. move this > createDatabase() call towards the top, with its callbacks defined below it. > Similarly, the callback defined in the getTransaction() call which starts the > next batch or calls cleanup could be a named function defined at the end of > runOneBatch so that "cleanUpFunc" is basically the last meaningful line in > testRandomReads(). I lean the opposite way, and always want everything defined before I use it [where recursion doesn't render that impossible]. Anybody else have a tie-breaking preference? https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_test.js:125: [testRandomReads, 1000, 5, 50, false], On 2012/07/26 18:23:30, jsbell wrote: > For readability, maybe define a "const" (var kUseIndex = true;) Good idea; done.
On 2012/07/26 20:24:11, ericu wrote: > > How about if I call it "getCompletionFunc" instead? It does all the completion > stuff, both cleanup and results-publishing. That sounds better to me. > I make it a consistent array everywhere so as to keep the type passed to > createDatabase consistent. I could check its type inside, but that seemed > messier. Ah, right, missed that you were re-using it (since the createDatabase call was so far away.) > I lean the opposite way, and always want everything defined before I use it > [where recursion doesn't render that impossible]. Anybody else have a > tie-breaking preference? Fight! Fight! Fight! :)
https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... File chrome/test/data/indexeddb/perf_test.js (right): https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... chrome/test/data/indexeddb/perf_test.js:117: createDatabase(testName, objectStoreNames, onCreated, onError, options); On 2012/07/26 20:24:11, ericu wrote: > On 2012/07/26 18:23:30, jsbell wrote: > > Might be just a personal thing, but I find tests more readable if the code > order > > follows the execution order, taking advantage of variable and function > hoisting > I lean the opposite way, and always want everything defined before I use it > [where recursion doesn't render that impossible]. Anybody else have a > tie-breaking preference? I like code order = execution order, possibly just because the IDB layout tests are written that way and I'm used to reading them. Alec, wdyt? I'm hoping for a 2v2 split...
On 2012/07/26 20:35:57, dgrogan wrote: > https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... > File chrome/test/data/indexeddb/perf_test.js (right): > > https://chromiumcodereview.appspot.com/10790041/diff/5001/chrome/test/data/in... > chrome/test/data/indexeddb/perf_test.js:117: createDatabase(testName, > objectStoreNames, onCreated, onError, options); > On 2012/07/26 20:24:11, ericu wrote: > > On 2012/07/26 18:23:30, jsbell wrote: > > > Might be just a personal thing, but I find tests more readable if the code > > order > > > follows the execution order, taking advantage of variable and function > > hoisting > > I lean the opposite way, and always want everything defined before I use it > > [where recursion doesn't render that impossible]. Anybody else have a > > tie-breaking preference? > > I like code order = execution order, possibly just because the IDB layout tests > are written that way and I'm used to reading them. > > Alec, wdyt? I'm hoping for a 2v2 split... Sorry I got buried in unread e-mail - just seeing this now, and I lean towards code order = execution order as well. I've found a lot of the tests that kind of run in "reverse" order confusing... |
