We used to check if a DB needs to be auto-vacuumed after every transaction. The check is freelist_count >= 10% * max_page_count. It turns out that getting freelist_count and max_page_count after every transaction adds a significant overhead. Once we changed the code to run this check only after transactions that have modified the database, the running times of our SELECT benchmarks have gone back to normal. We should improve this even further: we should get freelist_count and max_page_count only after transactions that have deleted something (DELETE, DROP TABLE, etc.). This should considerably reduce the auto-vacuum-related overhead for transactions that only insert or update data. This is a heuristic: in theory, an UPDATE could replace a huge BLOB with a NULL and reduce the size of a database by >= 10%, but it seems to me that this is a very uncommon case.
Created attachment 57050 [details] patch
Comment on attachment 57050 [details] patch Some drive by comments. WebCore/storage/DatabaseAuthorizer.cpp:311 + if (moduleName != "fts2") "fts2"... should this be "fts3"? WebCore/storage/SQLTransaction.cpp:476 + m_database->incrementalVacuumIfNeeded(); This changes the order in which 'vacuumIfNeeded' and 'didCommit' is called, I pretty sure didCommit involves reexaming the size of the database file for quota tracking purposes (which is why it's only called for xactions that modify the database). I think 'vacuumIfNeeded' should continue to be called first. WebCore/storage/DatabaseAuthorizer.cpp:325 + m_hadDeletes = m_hadDeletes || allow; x |= b is a nice shorthand
> WebCore/storage/DatabaseAuthorizer.cpp:311 > + if (moduleName != "fts2") > "fts2"... should this be "fts3"? fixed and landed this in another patch. > WebCore/storage/SQLTransaction.cpp:476 > + m_database->incrementalVacuumIfNeeded(); > This changes the order in which 'vacuumIfNeeded' and 'didCommit' is called, I pretty sure didCommit involves reexaming the size of the database file for quota tracking purposes (which is why it's only called for xactions that modify the database). > > I think 'vacuumIfNeeded' should continue to be called first. done. > WebCore/storage/DatabaseAuthorizer.cpp:325 > + m_hadDeletes = m_hadDeletes || allow; > x |= b is a nice shorthand isn't |= a binary operator? i don't think there's a similar boolean operator.
Created attachment 57601 [details] patch Same patch merged with other changes that were submitted; also, changed "m_hadDeletes = m_hadDeletes || allow" to "m_hadDeletes |= allow".
Comment on attachment 57601 [details] patch r=me, except for: > - return denyBasedOnTableName(tableName); > + bool allow = denyBasedOnTableName(tableName); > + m_hadDeletes |= allow; > + return allow; This looks like a neat helper function.
(In reply to comment #5) > (From update of attachment 57601 [details]) > r=me, except for: > > > - return denyBasedOnTableName(tableName); > > + bool allow = denyBasedOnTableName(tableName); > > + m_hadDeletes |= allow; > > + return allow; > > This looks like a neat helper function. done. added updateDeletesBasedOnTableName().
Landed as r60513.