IndexedDB 2.0: Cache and reuse SQLiteStatements in the SQLite backend

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

IndexedDB 2.0: Cache and reuse SQLiteStatements in the SQLite backend

sqlite_prepare_v2 is so hot that it's showing up at 15-20% on profiles of *one* specific test I'm working with.

Yikes!

Caching and re-using statements is an obvious win.

[Brady Eidson]

Created attachment 295640 [details]
Patch

[Brady Eidson]

This caught two patches in the upload, whoops. Sending only one now...

[Brady Eidson]

Created attachment 295642 [details]
Patch

[Alex Christensen]

Comment on attachment 295642 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=295642&action=review

r=me

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:2383
> +    return m_cachedStatements[static_cast<int>(sql)].get();

This should be bounds checked just to be extra careful, with an ASSERT_NOT_REACHED of some sort.
Also, size_t would be more appropriate than int.

[Build Bot]

Comment on attachment 295642 [details]
Patch

Attachment 295642 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2591094

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 295652 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Brady Eidson]

Those failures are due to the classic "w3c web server stopped working" issue.

[Brady Eidson]

https://trac.webkit.org/changeset/209096

[Brady Eidson]

This caused issues under Guardmalloc with deadlocked TransactionOperations.

Landing a stopgap fix until I can further explore why they aren't ref'ed

[Brady Eidson]

(In reply to comment #9)
> This caused issues under Guardmalloc with deadlocked TransactionOperations.
> 
> Landing a stopgap fix until I can further explore why they aren't ref'ed

Landed this in https://trac.webkit.org/changeset/209104

[Brady Eidson]

(In reply to comment #10)
> (In reply to comment #9)
> > This caused issues under Guardmalloc with deadlocked TransactionOperations.
> > 
> > Landing a stopgap fix until I can further explore why they aren't ref'ed
> 
> Landed this in https://trac.webkit.org/changeset/209104

This fix caused storage/indexeddb/modern/gc-closes-database.html and storage/indexeddb/modern/gc-closes-database-private.html to start timing out (I'm sure of why, also)

I'll skip them until I can resolve this mess tomorrow.

[Brady Eidson]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > This caused issues under Guardmalloc with deadlocked TransactionOperations.
> > > 
> > > Landing a stopgap fix until I can further explore why they aren't ref'ed
> > 
> > Landed this in https://trac.webkit.org/changeset/209104
> 
> This fix caused storage/indexeddb/modern/gc-closes-database.html and
> storage/indexeddb/modern/gc-closes-database-private.html to start timing out
> (I'm sure of why, also)
> 
> I'll skip them until I can resolve this mess tomorrow.

Skipped in https://trac.webkit.org/changeset/209114

Resolving the GuardMalloc *and* leak issue will happen over in bug https://bugs.webkit.org/show_bug.cgi?id=165186

[Chris Dumez]

Reverted r209096, r209104, and r209114 for reason:

Caused over 200 layout test failures on the bots

Committed r209117: <http://trac.webkit.org/changeset/209117>

[Chris Dumez]

Reverted r209096, r209104, and r209114 for reason:

Caused over 200 layout test failures on the bots

Committed r209117: <http://trac.webkit.org/changeset/209117>

[Chris Dumez]

(In reply to comment #14)
> Reverted r209096, r209104, and r209114 for reason:
> 
> Caused over 200 layout test failures on the bots
> 
> Committed r209117: <http://trac.webkit.org/changeset/209117>

E.g.
https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #14)
> > Reverted r209096, r209104, and r209114 for reason:
> > 
> > Caused over 200 layout test failures on the bots
> > 
> > Committed r209117: <http://trac.webkit.org/changeset/209117>
> 
> E.g.
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350

Note that https://trac.webkit.org/changeset/209104 did not help fix those layout test failures.

[Brady Eidson]

(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > > Reverted r209096, r209104, and r209114 for reason:
> > > 
> > > Caused over 200 layout test failures on the bots
> > > 
> > > Committed r209117: <http://trac.webkit.org/changeset/209117>
> > 
> > E.g.
> > https://build.webkit.org/builders/
> > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350
> 
> Note that https://trac.webkit.org/changeset/209104 did not help fix those
> layout test failures.

It wasn't meant to help fix those, which I didn't even know were happening.

It *was* meant to fix GuardMalloc failures from a previous changeset that I guess was now reintroduced.

It's somewhat surprising this change caused all that, but whatever - I'll sort it out in the A.M.

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > (In reply to comment #14)
> > > > Reverted r209096, r209104, and r209114 for reason:
> > > > 
> > > > Caused over 200 layout test failures on the bots
> > > > 
> > > > Committed r209117: <http://trac.webkit.org/changeset/209117>
> > > 
> > > E.g.
> > > https://build.webkit.org/builders/
> > > Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/11350
> > 
> > Note that https://trac.webkit.org/changeset/209104 did not help fix those
> > layout test failures.
> 
> It wasn't meant to help fix those, which I didn't even know were happening.
> 
> It *was* meant to fix GuardMalloc failures from a previous changeset that I
> guess was now reintroduced.

I rolled out all 3 changesets so I do not think I reintroduced failures but I could be wrong.

> 
> It's somewhat surprising this change caused all that, but whatever - I'll
> sort it out in the A.M.

Looks like the roll out was successful on the bots:
- Before: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19663
- After: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19664

The remaining unit test failure was caused by another change which was rolled out later on and we are green as of:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29/builds/19665

[Brady Eidson]

I can reproduce locally, only with the w3c tests, after a certain number of tests have run in a given process.

e.g.:
run-webkit-tests --child-processes=1 imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm --iterations=20000

Hammers the same test over and over, and after 68 runs it starts failing.

lsof confirms tons of handles to databases from past tests are still open, so opening new database files fails (limit to the number of file handles per process)

I'm still unclear how this change caused leaking file handles, but there's not much surface to go over.

[Brady Eidson]

(In reply to comment #19)
> I can reproduce locally, only with the w3c tests, after a certain number of
> tests have run in a given process.
> 
> e.g.:
> run-webkit-tests --child-processes=1
> imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm
> --iterations=20000
> 
> Hammers the same test over and over, and after 68 runs it starts failing.
> 
> lsof confirms tons of handles to databases from past tests are still open,
> so opening new database files fails (limit to the number of file handles per
> process)
> 
> I'm still unclear how this change caused leaking file handles, but there's
> not much surface to go over.

It's also quite unclear why this *doesn't* affect DRT, but only WKTR.  o_O

[Brady Eidson]

(In reply to comment #20)
> (In reply to comment #19)
> > I can reproduce locally, only with the w3c tests, after a certain number of
> > tests have run in a given process.
> > 
> > e.g.:
> > run-webkit-tests --child-processes=1
> > imported/w3c/web-platform-tests/IndexedDB/idbcursor_update_index3.htm
> > --iterations=20000
> > 
> > Hammers the same test over and over, and after 68 runs it starts failing.
> > 
> > lsof confirms tons of handles to databases from past tests are still open,
> > so opening new database files fails (limit to the number of file handles per
> > process)
> > 
> > I'm still unclear how this change caused leaking file handles, but there's
> > not much surface to go over.
> 
> It's also quite unclear why this *doesn't* affect DRT, but only WKTR.  o_O

DRT is leaking the handles, also. This must be "app process" vs "XPC service process" limitations.

DRT *does* start failing after enough runs (850 instead of 68)

[Brady Eidson]

Okay figured it out - If you close a sqlite3 handle while you still have prepared statements open, it leaks the file handles. o_O

Exact same patch with a couple of sqlite cleanup calls moved around fixes this.

[Brady Eidson]

Created attachment 295730 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295730 [details]
Patch

Clearing flags on attachment: 295730

Committed r209144: <http://trac.webkit.org/changeset/209144>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.