762083 - Reduce default places.sqlite size for profiles of WebApps

Status: RESOLVED FIXED

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

Description

[Clochix]

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.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Mak: what specifies the default size of places.sqlite?

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 5

[Marco Castelluccio [:marco]]

I think a pref would be simpler, Marco can we do that?

Comment 6

[Marco Bonardo [:mak]]

(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.

Comment 7

[Marco Castelluccio [:marco]]

(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.

Comment 8

[Marco Castelluccio [:marco]]

Marco, do you want to work on this? Otherwise, may you point me in the right direction?

Comment 9

[Marco Bonardo [:mak]]

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.

Comment 10

[Marco Castelluccio [:marco]]

Attachment: avoidPlacesGrowth

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).

Comment 11

[Marco Castelluccio [:marco]]

Try looks green: https://tbpl.mozilla.org/?tree=Try&rev=17ec6c756c49

Comment 12

[Marco Bonardo [:mak]]

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

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Patch

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/70811e50312c

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70811e50312c

Comment 16

[rsx11m]

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

[rsx11m]

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.

Comment 18

[Marco Castelluccio [:marco]]

Thank you for noticing! I'll fix this problem tomorrow.

Comment 19

[Marco Castelluccio [:marco]]

Attachment: Followup fix

Comment 20

[Marco Bonardo [:mak]]

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

Comment 21

[Marco Castelluccio [:marco]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28a4d4ae7b1f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28a4d4ae7b1f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/979e501cd374