445525 - set SQLITE_CONFIG_MEMSTATUS to 0

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Shawn Wilsher :sdwilsh]

From drh:
The statistics just record the current and maximum memory usage.  Some applications need that - but typically they are on embedded hardware where memory is scarce.  I don't imagine they are important for a workstation app like FF.

It'll save us some mutexing.

Comment 1

[Shawn Wilsher :sdwilsh]

More info: it saves a single mutex enter and leave for each malloc.

Comment 2

[Shawn Wilsher :sdwilsh]

Andrew - would you be interested in tackling this by chance?  Should speed up many queries.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

You have no idea how much willpower it required to avoid a Top Gun reference.

This patch introduces an explicit call to sqlite3_initialize so it's very clear when we are calling things before initialization and when we are not.

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 345087 [details] [diff] [review]
v1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

>diff --git a/storage/src/mozStorageService.cpp b/storage/src/mozStorageService.cpp
>+    // Disable memory allocation statistic collection, improving performance.
>+    // This must be done prior to a call to sqlite3_initialize to have any
>+    // effect.
>+    int rc = sqlite3_config(SQLITE_CONFIG_MEMSTATUS, 0);
>+    // We want to alert people if we are somehow already initialized in debug
>+    // builds, but this does not pose an operational problem.
>+    NS_ASSERTION(rc == SQLITE_OK, "SQLite should not be initialized yet.");
I think we should actually propagate that error and not let storage initialize if it returns an error.  (getting the service will throw)

>+    // Explicitly initialize sqlite3.  Although this is implicitly called by
>+    // various sqlite3 functions (and the sqlite3_open calls in our case),
>+    // the documentation suggests calling this directly.  So we do.
>+    sqlite3_initialize();
nit: cast return value to void since we do not care about the result.

We should also be calling sqlite_shutdown in our destructor at this point.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

Points addressed.  I also modified the code to check the return value of sqlite3_initialize and propagate failure rather than ignoring it.  The shutdown warns on failure but can't do anything else, being part of a destructor call.  Tests pass.

Comment 6

[Shawn Wilsher :sdwilsh]

Comment on attachment 345183 [details] [diff] [review]
v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

r=sdwilsh

Thanks for doing this!

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

This passed linux (talos too) and OS X try-server builds, marking for check-in.  (The windows build had unrelated bustage, and the windows talos didn't even try to run it.  Given the small nature of the change and the fact that it takes the try-servers an unreasonable amount of time to get around to things, I'm not going to push again just to try and get a windows success.)

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345183 [details] [diff] [review]
v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

We'll wait for after beta for this.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 345183 [details] [diff] [review]
v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

a191=beltzner

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 345183 [details] [diff] [review]
v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

Sorry:
{
storage/src/mozStorageService.cpp
Hunk #1 FAILED at 68
1 out of 1 hunks FAILED
}

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time. de-bitrotted, against trunk

dang, I was hoping to be able to keep the 1.9.1 approval flag if we were obsoleting the former patch.  This is just the same patch de-bitrotted against trunk.  I understand this to need to hit the trunk first, then it can land on 1.9.1 after baking.  The trunk is orange, so I can't land this now, though.

Carrying forward r=sdwilsh and conceptually carrying forward a191=beltzner.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Attachment: [as landed on trunk] v2 turn off SQLITE_CONFIG_MEMSTATUS at init-time, update (from bustage fix) windows lib def stuff

Landed in http://hg.mozilla.org/mozilla-central/rev/7d7a0c514970
Bustage fix: http://hg.mozilla.org/mozilla-central/rev/5fbf1ca2b2ef

The patch was missing the .def stuff; once I get around the catch-22 where I need windows to download my windows images from MSDN, that should hopefully not happen again.  This patch has that stuff updated.

Not sure what to do about the approval flag, so requesting it again (we already had approval for the bit-rotted (in context) version without the windows .def stuff).  Carrying forward r=sdwilsh for the actual review.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Marking fixed since it's in trunk.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 352892 [details] [diff] [review]
[as landed on trunk] v2 turn off SQLITE_CONFIG_MEMSTATUS at init-time, update (from bustage fix) windows lib def stuff

You don't need approval for the bustage fix.

Comment 15

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4e3b01c9262e
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/06f8f373eb7a