Closed
Bug 762083
Opened 13 years ago
Closed 11 years ago
Reduce default places.sqlite size for profiles of WebApps
Categories
(Firefox Graveyard :: Web Apps, enhancement, P1)
Tracking
(firefox16 wontfix, firefox28 fixed, firefox29 fixed)
RESOLVED
FIXED
Firefox 29
People
(Reporter: clochix, Assigned: marco)
References
Details
(Whiteboard: [qa-])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mak
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Each Web App has its own profile. It contains the places.sqlite database whose default size is 10Mo to prevent fragmentation (see #581606 ).
So each app create a directory that weighs at least 14Mo.
I don't know if Apps are gonna use a lot places.sqlite, so maybe the fragmentation issue is irrelevant in this case and the new profile could contain a smaller database.
Updated•13 years ago
|
Hardware: x86 → All
Comment 1•13 years ago
|
||
Mak: what specifies the default size of places.sqlite?
Comment 2•13 years ago
|
||
IIRC I discussed this with Felipe during the last work week, but we forgot to file the bug. The problem is chunked-growth, the databases (places, cookies and urlclassifier) are setup to grow in chunks. For Places the chunk size is 10MB, for urlclassifier it's 5MB, for cookies is 500KB.
See mxr.mozilla.org/mozilla-central/search?string=SetGrowthIncrement
For WebApps we clearly don't want this.
There are 2 alternatives though, either we disable chunked growth in Storage when we build the runtime, or we disable it separately for each consumer. It depends if for example we may want to retain the cookies growth for apps performances (not that I think matters so much for a single app) or if in future we may need it for something else.
Which clever instruments do we have to detect if the runtime is a webapp? Otherwise, we may even go with a pref.
Updated•13 years ago
|
OS: Linux → All
Priority: -- → P2
Comment 3•13 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #2)
>
> Which clever instruments do we have to detect if the runtime is a webapp?
> Otherwise, we may even go with a pref.
One way to do is this: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#15
(I don't know offhand what is the equivalent way to get this in C++)
I thought also about XRE_GetProcessType() but it is the same for firefox and the runtime, probably intentionally as changing that from GeckoProcessType_Default would have more complex implications
A pref is also easy to add as the runtime has its own prefs, http://mxr.mozilla.org/mozilla-central/source/webapprt/prefs.js
Updated•12 years ago
|
QA Contact: jsmith
Updated•12 years ago
|
status-firefox16:
--- → wontfix
Assignee | ||
Comment 5•11 years ago
|
||
I think a pref would be simpler, Marco can we do that?
Flags: needinfo?(mak77)
Comment 6•11 years ago
|
||
(In reply to Marco Castelluccio [:marco] from comment #5)
> I think a pref would be simpler, Marco can we do that?
sure, the remaining question is whether it should be done as a Storage pref (that affects all consumers) or a separate perf per service, that so far would be one for Places and one for Cookies.
Off-hand I think the latter would be better, we could have a pref defining the wanted chunked growth size, in kibibytes. If set to 0, it will basically disable the feature. Something like places.database.growthIncrementKiB? for Firefox it should be set to 10MiB, while for webapps should be set to 0.
Probably we don't care much about the 512KB growth in cookies.
Flags: needinfo?(mak77)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #6)
> sure, the remaining question is whether it should be done as a Storage pref
> (that affects all consumers) or a separate perf per service, that so far
> would be one for Places and one for Cookies.
> Off-hand I think the latter would be better, we could have a pref defining
> the wanted chunked growth size, in kibibytes. If set to 0, it will basically
> disable the feature. Something like places.database.growthIncrementKiB? for
> Firefox it should be set to 10MiB, while for webapps should be set to 0.
> Probably we don't care much about the 512KB growth in cookies.
I agree, the latter is better.
Updated•11 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 8•11 years ago
|
||
Marco, do you want to work on this? Otherwise, may you point me in the right direction?
Flags: needinfo?(mak77)
Comment 9•11 years ago
|
||
I think you may just add places.database.growthIncrementKiB pref, set to 10MB in Firefox and Seamonkey, it's unlikely other consumers like thunderbird or webapps benefit from it, so I'd keep the default (if the pref is missing) to 0, that means don't grow.
Flags: needinfo?(mak77)
Assignee | ||
Comment 10•11 years ago
|
||
Still waiting for try results.
There's probably still room for improvements in other fields (for example the cache is > 10 MB as soon as it's created).
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8335678 -
Flags: review?(mak77)
Assignee | ||
Comment 11•11 years ago
|
||
Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=17ec6c756c49
Comment 12•11 years ago
|
||
Comment on attachment 8335678 [details] [diff] [review]
avoidPlacesGrowth
Review of attachment 8335678 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/places/Database.cpp
@@ +595,5 @@
> nsAutoCString journalSizePragma("PRAGMA journal_size_limit = ");
> journalSizePragma.AppendInt(DATABASE_MAX_WAL_SIZE_IN_KIBIBYTES * 3);
> (void)mMainConn->ExecuteSimpleSQL(journalSizePragma);
>
> + nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
nit: I prefer the " = " style
@@ +599,5 @@
> + nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
> + int32_t growthIncrementKiB = 10 * BYTES_PER_MEBIBYTE;
> + if (prefs) {
> + prefs->GetIntPref(PREF_GROWTH_INCREMENT_KIB, &growthIncrementKiB);
> + }
I'd prefer if you'd use
int32_t growthIncrementKiB = Preferences::GetInt ... and provide our default as second argument (you can remove BYTES_PER_MEBIBYTE and express everything in KiB)
@@ +603,5 @@
> + }
> +
> + // Grow places in |growthIncrement| increments to limit fragmentation on disk.
> + // By default, it's 10 MB.
> + if (growthIncrementKiB != 0) {
well, > 0 is a stricter check and makes more sense since we start from a signed int
Attachment #8335678 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8335678 -
Attachment is obsolete: true
Attachment #8337141 -
Flags: review?(mak77)
Updated•11 years ago
|
Attachment #8337141 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 15•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
Comment 16•11 years ago
|
||
Comment on attachment 8337141 [details] [diff] [review]
Patch
(Quoting rsx11m from bug 672988 comment #12)
> On a side note, without that patch to set the preference, the default of
> 10MB should be maintained. However, in that case I only get 1179648 bytes,
> which is 32KB more (even) compared with the preference setting set to zero.
Assuming that this is reproducible in a Firefox trunk build, I think that the problem is caused by applying BYTES_PER_KIBIBYTE three times instead of just twice:
>+ int32_t growthIncrementKiB =
>+ Preferences::GetInt(PREF_GROWTH_INCREMENT_KIB, 10 * BYTES_PER_KIBIBYTE * BYTES_PER_KIBIBYTE);
This should result in 10485760 as desired, but the unit is KiB not just bytes, hence
>+ if (growthIncrementKiB > 0) {
>+ (void)mMainConn->SetGrowthIncrement(growthIncrementKiB * BYTES_PER_KIBIBYTE, EmptyCString());
>+ }
would make it SetGrowthIncrement(10737418240 bytes), or not?
Taken modulo 2^32 for a signed 32 bit integer, this produces a negative value and may explain the lack of effect of the default to yield the desired 10MB increments.
Comment 17•11 years ago
|
||
Confirmed in Firefox and thus reopening. The December 2nd Nightly 28.0a1 build produces a 10240KB places.sqlite file in a new profile, the December 3rd Nightly 1152KB without that preference set. Setting places.database.growthIncrementKiB to 10240 yields the desired 10MB default file/chunk size.
This should be an easy fix, just make it 10 * BYTES_PER_KIBIBYTE for the fallback value in the initial growthIncrementKiB assignment.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•11 years ago
|
||
Thank you for noticing! I'll fix this problem tomorrow.
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #8343792 -
Flags: review?(mak77)
Comment 20•11 years ago
|
||
Comment on attachment 8343792 [details] [diff] [review]
Followup fix
Review of attachment 8343792 [details] [diff] [review]:
-----------------------------------------------------------------
whoops, please request approval for FF28 since this may be a perf issue for some profiles
Attachment #8343792 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 8343792 [details] [diff] [review]
Followup fix
[Approval Request Comment]
Bug caused by (feature/regressing bug #): The first patch in this bug (the default value for the new pref wasn't correct)
User impact if declined: Too big places database
Risk to taking this patch (and alternatives if risky): No risk, it just changes the default pref value to what it was meant to be
String or IDL/UUID changes made by this patch: None
Attachment #8343792 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Comment 22•11 years ago
|
||
Keywords: checkin-needed
Comment 23•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Updated•11 years ago
|
Attachment #8343792 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•11 years ago
|
||
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•