(#6055) - remove docCount optimization from IDB #6056
Conversation
| var error = new Error('db isn\'t open'); | ||
| error.id = 'idbNull'; | ||
| return callback(error); | ||
| } |
There was a problem hiding this comment.
Setting idb to null was not necessary; we can just check cachedDBs.
Also, there is no need to manually check for closed state because adapter.js already does that.
| }); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I ran into a quasi-unrelated bug with closed so I wanted more tests to confirm it was working correctly.
|
|
||
| after(function () { | ||
| worker.terminate(); | ||
| }); |
There was a problem hiding this comment.
creating and terminating a new worker for every test here was really not necessary
a4e1237 to
0ea6baa
Compare
Removes this unsafe optimization.
|
Ran some measurements using #6067. This is a regression on the Chrome: 631ms -> 1081ms My guess is that Firefox is caching the count somehow under the hood (or SQLite is just very smart about this), whereas Chrome isn't. Regardless, this PR is the correct behavior, and it's better to be correct than to be fast. My tests in #6060 indicate that we can still get some wins, although I don't know if we can get Chrome to be as fast as it used to be. |
|
It's also a modest perf regression in Edge 15 (beta): 1717ms -> 1517ms |
|
Being correct > Being fast |
Adds a new test to
browser.worker.js. The test fails before the fix but succeeds after, demonstrating the issue withdocCount.I'll need to run some perf tests to see what kind of impact this has, but I believe that since we can now use
getAll()-based cursors forallDocs(), we can offset whatever perf regression this causes. Also it's the right thing to do because it's more correct.I will have to implement the same fix for WebSQL (due to the cross-tab issue) but I'll do that in another PR. The worker test does not run for WebSQL because no modern browser supports WebSQL in a worker.