1346554 - Use PRAGMA incremental_vacuum on favicons.sqlite

Status: RESOLVED FIXED

(Toolkit :: Places, enhancement, P1)

Description

[Marco Bonardo [:mak]]

the db will support incremental vacuum, we need a strategy to use it, either on idle or after large removals.

Comment 1

[Marco Bonardo [:mak]]

Attachment: data-review.txt

Comment 2

[Marco Bonardo [:mak]]

Attachment: Bug 1346554 - Incremental vacuum favicons.sqlite.

Review commit: https://reviewboard.mozilla.org/r/214320/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214320/

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8943977 [details]
data-review.txt

r+ with some nits.

Does this need to be permanent data collection? Would setting a 6mo expiry to revisit whether this data is useful/needs to be collected be acceptable?

For permanent data collection, please include a human individual's email for alert emails.

---

Data-review only

> Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, Histograms.json

> Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Yes, Firefox data controls

> If the request is for permanent data collection, is there someone who will monitor the data over time?**

No - [e-mail] but no individual email included. Requested in data review.

> Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under? **

Type 1, technical data

> Is the data collection request for default-on or default-off?
Default off for release

> Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No

> Is the data collection covered by the existing Firefox privacy notice?

Yes

> Does there need to be a check-in in the future to determine whether to renew the data? (Yes/No) (If yes, set a todo reminder or file a bug if appropriate)**

Establishing a baseline for Favicons db size in Places - can be permanent bc also opt-in for release

Comment 4

[Chenxia Liu [:liuche]]

Comment on attachment 8943991 [details]
Bug 1346554 - Incremental vacuum favicons.sqlite.

https://reviewboard.mozilla.org/r/214320/#review220416

::: toolkit/components/telemetry/Histograms.json:5407
(Diff revision 1)
>      "n_buckets": 20,
>      "description": "PLACES: Average size of a place in the database (bytes)"
>    },
> +  "PLACES_DATABASE_FAVICONS_FILESIZE_MB": {
> +    "record_in_processes": ["main"],
> +    "alert_emails": ["[e-mail]"],

Please include an individual's email, rather than just a mailing list.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Chenxia Liu [:liuche] - not actively working on Fennec from comment #3)
> Does this need to be permanent data collection? Would setting a 6mo expiry
> to revisit whether this data is useful/needs to be collected be acceptable?

We'd prefer it to be permanent because we'll use it to check for regressions we may not notice in other ways. We have a similar probe on places.sqlite. This also depends on third party libraries (Sqlite). Every time we modify the schema, contents or expiration policies, these numbers can vary and we must be able to identify whether the change was inside an acceptable bracket.

> For permanent data collection, please include a human individual's email for
> alert emails.

Is this a strict requirement?
While I have no problems putting myself as a reference, I honestly find more useful to set a whole team as responsible for probes, because I may be unavailable (PTO, sickness, whatever else) when an emergency arises, and then nobody may notice it.
fx-search is the working list for the fx-search team that I'm part of. Everyone on that list is responsible for these probes.
I was indeed not sure why we weren't doing the same with fx-team and their relevant probes, depending on a single person, vs a team, sounds more dangerous.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8943991 [details]
Bug 1346554 - Incremental vacuum favicons.sqlite.

https://reviewboard.mozilla.org/r/214320/#review220888

Looks good. r=Standard8 once the data review is sorted.

::: toolkit/components/places/PlacesDBUtils.jsm:152
(Diff revision 1)
>      return logs;
>    },
>  
>    async invalidateCaches() {
>      let logs = [];
> -    try {
> +    return PlacesUtils.withConnectionWrapper("PlacesDBUtils: invalidate caches", async db => {

You could potentially drop the async definition on invalidateCaches() now, but I'm fine either way.

::: toolkit/components/places/tests/favicons/test_incremental_vacuum.js:6
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests incremental vacuum of the favicons database.
> +
> +Components.utils.import("resource://gre/modules/PlacesDBUtils.jsm");

Should be able to use Cu here.

Comment 7

[Chenxia Liu [:liuche]]

Hi Mak, you can have multiple emails in the alert emails - having a list is good, but also including an individual email makes it clear who is responsible for monitoring and using the data being collected.

Comment 8

[Marco Bonardo [:mak]]

oh right, thanks! I will add both.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8943991 [details]
Bug 1346554 - Incremental vacuum favicons.sqlite.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214320/diff/1-2/

Comment 10

[Code Review Bot [:reviewbot]]

Comment on attachment 8943991 [details]
Bug 1346554 - Incremental vacuum favicons.sqlite.

https://reviewboard.mozilla.org/r/214320/#review221254


Static analysis found 1 defect in this patch.
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/places/Database.cpp:1978
(Diff revision 2)
> +  MOZ_ASSERT(NS_IsMainThread());
> +  // auto_vacuum of the favicons database was broken, we may have to set it again.
> +  int32_t vacuum = 0;
> +  {
> +    nsCOMPtr<mozIStorageStatement> stmt;
> +    nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(

Warning: Value stored to 'rv' during its initialization is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8943991 [details]
Bug 1346554 - Incremental vacuum favicons.sqlite.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214320/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a39f7d5c0041
Incremental vacuum favicons.sqlite. r=standard8

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/a39f7d5c0041