Closed Bug 762083 Opened 8 years ago Closed 6 years ago

Reduce default places.sqlite size for profiles of WebApps

Categories

(Firefox Graveyard :: Web Apps, enhancement, P1)

16 Branch
enhancement

Tracking

(firefox16 wontfix, firefox28 fixed, firefox29 fixed)

RESOLVED FIXED
Firefox 29
Tracking Status
firefox16 --- wontfix
firefox28 --- fixed
firefox29 --- fixed

People

(Reporter: clochix, Assigned: marco)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

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.
Hardware: x86 → All
Mak: what specifies the default size of places.sqlite?
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.
OS: Linux → All
Priority: -- → P2
(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
QA Contact: jsmith
Duplicate of this bug: 850615
I think a pref would be simpler, Marco can we do that?
Flags: needinfo?(mak77)
(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)
(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.
Priority: P2 → P1
Marco, do you want to work on this? Otherwise, may you point me in the right direction?
Flags: needinfo?(mak77)
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)
Attached patch avoidPlacesGrowth (obsolete) — Splinter Review
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)
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+
Attached patch PatchSplinter Review
Attachment #8335678 - Attachment is obsolete: true
Attachment #8337141 - Flags: review?(mak77)
Attachment #8337141 - Flags: review?(mak77) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/70811e50312c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
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.
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 → ---
Thank you for noticing! I'll fix this problem tomorrow.
Blocks: 672988
Attached patch Followup fixSplinter Review
Attachment #8343792 - Flags: review?(mak77)
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+
Keywords: checkin-needed
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?
Target Milestone: Firefox 28 → ---
https://hg.mozilla.org/mozilla-central/rev/28a4d4ae7b1f
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Attachment #8343792 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.