Closed Bug 445525 Opened 12 years ago Closed 12 years ago

set SQLITE_CONFIG_MEMSTATUS to 0

Categories

(Toolkit :: Storage, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: sdwilsh, Assigned: asuth)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

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.
No longer depends on: 445042
Depends on: 449443
More info: it saves a single mutex enter and leave for each malloc.
Andrew - would you be interested in tackling this by chance?  Should speed up many queries.
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.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
Attachment #345087 - Flags: review?(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.
Attachment #345087 - Flags: review?(sdwilsh) → review-
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.
Attachment #345087 - Attachment is obsolete: true
Attachment #345183 - Flags: review?(sdwilsh)
Comment on attachment 345183 [details] [diff] [review]
v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

r=sdwilsh

Thanks for doing this!
Attachment #345183 - Flags: review?(sdwilsh) → review+
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.)
Keywords: checkin-needed
Attachment #345183 - Flags: approval1.9.1b2?
Keywords: checkin-needed
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.
Attachment #345183 - Flags: approval1.9.1b2? → approval1.9.1b2-
Comment on attachment 345183 [details] [diff] [review]
v1.1 turn off SQLITE_CONFIG_MEMSTATUS at init-time

a191=beltzner
Attachment #345183 - Flags: approval1.9.1+
Keywords: checkin-needed
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
}
Attachment #345183 - Attachment is obsolete: true
Attachment #345183 - Attachment is obsolete: false
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.
Attachment #345183 - Attachment is obsolete: true
Attachment #352497 - Flags: review+
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.
Attachment #352497 - Attachment is obsolete: true
Attachment #352892 - Flags: review+
Attachment #352892 - Flags: approval1.9.1?
Marking fixed since it's in trunk.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [baking on trunk]
Attachment #352892 - Flags: approval1.9.1?
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.
Target Milestone: --- → mozilla1.9.2a1
You need to log in before you can comment on or make changes to this bug.