Bug 39688 – We should check if a DB needs to be auto-vacuumed only after transactions that delete something

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

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.

[Dumitru Daniliuc]

Created attachment 57050 [details]
patch

[Michael Nordman]

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

[Dumitru Daniliuc]

> 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.

[Dumitru Daniliuc]

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".

[Dimitri Glazkov (Google)]

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.

[Dumitru Daniliuc]

(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().

[Dumitru Daniliuc]

Landed as r60513.