Disable sqlite lookaside buffers by default.

Disable sqlite lookaside buffers by default.

Sqlite by default allocates 50KB per connection for use as a lookaside buffer
for small object allocation. With the malloc implementations we use, this
doesn't seem to help gain us any performance advantage, while increasing
memory overhead, therefore this CL disables sqlite lookaside buffers by default.

BUG=chromium:243769
TEST=Ran page_cycler perf test in Linux and Android and saw no noticable
     degradation. Examined traceview didn't see an increase in the time spent
     on the history or database threads.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204623

[Scott Hess - ex-Googler, 2013-06-04 16:35:21]

I think adding SQLITE_OMIT_LOOKASIDE to the defines in sqlite.gyp would make
more sense.  Then it can easily be tweaked per-platform, for some definition of
easy.  If it turns out to be too broad, we can later circle back and figure out
what the trigger should be.  Though any platform having problems with these
kinds of allocations is already going to have a poor time with Chromium ...

https://codereview.chromium.org/16356007/diff/2001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/16356007/diff/2001/sql/connection.h#newcode152
sql/connection.h:152: 
IMHO, this is either the right thing to do, or not the right thing to do, though
it's possible that the choice differs per-platform.  So exposing this seems like
it's asking for trouble.

[rmcilroy, 2013-06-04 17:05:38]

Sounds sensible.  I've updated patch to use SQLITE_OMIT_LOOKASIDE in sql.gyp. 
Is this how you want it the define (I'm new to gyp)?

https://codereview.chromium.org/16356007/diff/2001/sql/connection.h
File sql/connection.h (right):

https://codereview.chromium.org/16356007/diff/2001/sql/connection.h#newcode152
sql/connection.h:152: 
On 2013/06/04 16:35:21, shess wrote:
> IMHO, this is either the right thing to do, or not the right thing to do,
though
> it's possible that the choice differs per-platform.  So exposing this seems
like
> it's asking for trouble.

Done.

[Scott Hess - ex-Googler, 2013-06-04 17:20:35]

Apologies, I could have been more specific.

Put the SQLITE_OMIT_LOOKASIDE define down in third_party/sqlite/sqlite.gyp's
defines section.  Then the lookaside code will be dropped from sqlite entirely,
and it will also benefit non-sql/ users of SQLite (like webdb).

Looping in Pawel as a FYI, in case he thinks that this might interact poorly
with USE_SYSTEM_SQLITE.  I _think_ that if the system has a good malloc library,
it should include this OMIT with the system build of SQLite, rather than having
each app make that decision.  AFAICT the OMIT isn't referenced in any SQLite
header files, so things should be fine from that end.

[Paweł Hajdan Jr., 2013-06-04 18:11:04]

This is fine for me. System sqlite is blocked on much harder things anyway,
like... WebSQL!

[rmcilroy, 2013-06-05 13:26:29]

On 2013/06/04 17:20:35, shess wrote:
> Apologies, I could have been more specific.
> 
> Put the SQLITE_OMIT_LOOKASIDE define down in third_party/sqlite/sqlite.gyp's
> defines section.  Then the lookaside code will be dropped from sqlite
entirely,
> and it will also benefit non-sql/ users of SQLite (like webdb).
> 
> Looping in Pawel as a FYI, in case he thinks that this might interact poorly
> with USE_SYSTEM_SQLITE.  I _think_ that if the system has a good malloc
library,
> it should include this OMIT with the system build of SQLite, rather than
having
> each app make that decision.  AFAICT the OMIT isn't referenced in any SQLite
> header files, so things should be fine from that end.

That makes sense - done.  Unfortunately it seems that sqlite still allocates the
lookaside buffer even when SQLITE_OMIT_LOOKASIDE is defined.  I've patched
sqlite to fix this in this patch-set.  I will send omit-lookaside.patch upstream
to sqlite so we can eventually get rid of it as a local patch if you agree that
this looks fine.

[Scott Hess - ex-Googler, 2013-06-05 17:43:22]

On 2013/06/05 13:26:29, Ross Mcilroy wrote:
> On 2013/06/04 17:20:35, shess wrote:
> > Apologies, I could have been more specific.
> > 
> > Put the SQLITE_OMIT_LOOKASIDE define down in third_party/sqlite/sqlite.gyp's
> > defines section.  Then the lookaside code will be dropped from sqlite
> entirely,
> > and it will also benefit non-sql/ users of SQLite (like webdb).
> > 
> > Looping in Pawel as a FYI, in case he thinks that this might interact poorly
> > with USE_SYSTEM_SQLITE.  I _think_ that if the system has a good malloc
> library,
> > it should include this OMIT with the system build of SQLite, rather than
> having
> > each app make that decision.  AFAICT the OMIT isn't referenced in any SQLite
> > header files, so things should be fine from that end.
> 
> That makes sense - done.  Unfortunately it seems that sqlite still allocates
the
> lookaside buffer even when SQLITE_OMIT_LOOKASIDE is defined.  I've patched
> sqlite to fix this in this patch-set.  I will send omit-lookaside.patch
upstream
> to sqlite so we can eventually get rid of it as a local patch if you agree
that
> this looks fine.

OK.  So my assumption was that SQLITE_OMIT_LOOKASIDE was like other
SQLITE_OMIT_* cases, where it not only disables the feature, but comprehensively
removes all of the overhead code associated with the feature.  But, AFAICT,
SQLITE_OMIT_LOOKASIDE just disables the feature, and more-current versions of
SQLite still work that way.  I find this more than a little annoying.

Your new change is right WRT patching SQLite, but if SQLite doesn't do the right
thing WRT SQLITE_OMIT_LOOKASIDE, I'd rather just go back to patch #2.  Except
make it the default without a flag to set things.

Thanks for sticking with this through the runaround.  It's "only" 50k, but,
well, probably like a terabyte of memory in aggregate across the userbase :-).

[Scott Hess - ex-Googler, 2013-06-05 17:58:48]

https://codereview.chromium.org/16356007/diff/2001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/16356007/diff/2001/sql/connection.cc#newcode637
sql/connection.cc:637: sqlite3_db_config(db_, SQLITE_DBCONFIG_LOOKASIDE, NULL,
0, 0);
And maybe buff up the first line of the comment to indicate why it's being done.
 On the order of "SQLite uses a lookaside buffer to improve performance of small
mallocs.  Chromium already depends on small mallocs being efficient, so disable
this."  Or something like that.

[rmcilroy, 2013-06-05 22:24:39]

> OK.  So my assumption was that SQLITE_OMIT_LOOKASIDE was like other
> SQLITE_OMIT_* cases, where it not only disables the feature, but
comprehensively
> removes all of the overhead code associated with the feature.  But, AFAICT,
> SQLITE_OMIT_LOOKASIDE just disables the feature, and more-current versions of
> SQLite still work that way.  I find this more than a little annoying.

Agreed, this is super annoying behaviour (which took a while to figure out why
the allocations weren't going away with SQLITE_OMIT_LOOKASIDE.

> Your new change is right WRT patching SQLite, but if SQLite doesn't do the
right
> thing WRT SQLITE_OMIT_LOOKASIDE, I'd rather just go back to patch #2.  Except
> make it the default without a flag to set things.

Done

> Thanks for sticking with this through the runaround.  It's "only" 50k, but,
> well, probably like a terabyte of memory in aggregate across the userbase :-).

Actually it's closer to 500KB since there are around 8 connections open, so
let's go for petabytes across the user base ;).

[rmcilroy, 2013-06-05 22:24:47]

https://codereview.chromium.org/16356007/diff/2001/sql/connection.cc
File sql/connection.cc (right):

https://codereview.chromium.org/16356007/diff/2001/sql/connection.cc#newcode637
sql/connection.cc:637: sqlite3_db_config(db_, SQLITE_DBCONFIG_LOOKASIDE, NULL,
0, 0);
On 2013/06/05 17:58:49, shess wrote:
> And maybe buff up the first line of the comment to indicate why it's being
done.
>  On the order of "SQLite uses a lookaside buffer to improve performance of
small
> mallocs.  Chromium already depends on small mallocs being efficient, so
disable
> this."  Or something like that.

Done.

[Scott Hess - ex-Googler, 2013-06-05 22:32:03]

On 2013/06/05 22:24:39, Ross Mcilroy wrote:
> > Thanks for sticking with this through the runaround.  It's "only" 50k, but,
> > well, probably like a terabyte of memory in aggregate across the userbase
:-).
> 
> Actually it's closer to 500KB since there are around 8 connections open, so
> let's go for petabytes across the user base ;).

I probably have the magnitudes of my tera and peta inverted!

LGTM.

[commit-bot: I haz the power, 2013-06-06 13:22:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmcilroy@chromium.org/16356007/16001

[commit-bot: I haz the power, 2013-06-06 22:20:35]

Change committed as 204623