2007091 - Clearing favicons from sqlite takes lots of time and cpu resources

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Olli Pettay [:smaug][[e-mail]]]

The following sqlite query uses tons of CPU time and makes the laptop fan scream for quite some time
DELETE FROM moz_icons_to_pages
WHERE (page_id, icon_id) IN (
SELECT page_id, icon_id
FROM moz_icons_to_pages ip
JOIN moz_icons i ON i.id = icon_id
JOIN moz_pages_w_icons pi ON pi.id = page_id
JOIN moz_places ON url_hash = page_url_hash
WHERE
last_visit_date BETWEEN
strftime('%s', ip.expire_ms / 1000, 'unixepoch', '+6 months', 'localtime', 'utc') * 1000000
AND strftime('%s', 'now', 'localtime', '-6 months', 'utc') * 1000000

I think it was added in bug 1959694.

https://share.firefox.dev/4pGdQHv

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1959694

Comment 2

[Marco Bonardo [:mak] (Away 10-21 August)]

Ok, I see some entries in SlowSQL telemetry, so we can check after fixes this is not an issue anymore.
I see ~3000 users affected, both in 147 and 148.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

[Tracking Requested - why for this release]:
High CPU use for significant amount time.

Comment 4

[Marco Bonardo [:mak] (Away 10-21 August)]

I think there is a bug in the SQLite query optimizer, for example I see that using an index on root it is returning the wrong number of results... and it's pretty slower. Skipping the root index seems to do the right thing and it's faster. Though I should first create a small use case to report the issue upstream.
In the meanwhile, could you please try running this query in the Browser Console? By not using the root index it should be significantly faster, it's like 6 times faster here.

(async function() {
  let conn = await PlacesUtils.promiseDBConnection();
  let time = performance.now();
  let rows = await conn.execute(`SELECT count(*) FROM moz_icons_to_pages
    WHERE (page_id, icon_id) IN (
      SELECT page_id, icon_id
      FROM moz_icons_to_pages ip
      JOIN moz_pages_w_icons pi ON pi.id = page_id
      JOIN moz_places ON url_hash = page_url_hash
      WHERE
      last_visit_date BETWEEN
        strftime('%s', ip.expire_ms / 1000, 'unixepoch', '+6 months', 'localtime', 'utc') * 1000000
        AND strftime('%s', 'now', 'localtime', '-6 months', 'utc') * 1000000
      AND foreign_count = 0
      AND NOT EXISTS (SELECT 1 FROM moz_icons WHERE id = icon_id AND root = 1)
	  LIMIT 50
    )
`);
time = performance.now() - time;
console.log(`Results: ${rows[0].getResultByIndex(0)} in ${time}`);
})()

Comment 5

[Marco Bonardo [:mak] (Away 10-21 August)]

Oh, sorry, I forgot the needinfo

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Results: 50 in 5.1294269999998505

Comment 7

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: Bug 2007091 - Workaround a SQLite query optimization bug. r=daisuke,jteow

SQLite is optimizing this expiration query wrongly, when using a
bloom filter and an automatic covering index on moz-icons.root.
In addition we're removing the ORDER BY clause as it's expensive. While
it makes sense to first expire older entries, in this case all the
entries are old, so we can save some power by not ordering them.
Also reduce the LIMIT to 50, for smaller I/O operations.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/294d79d1999a
https://hg.mozilla.org/integration/autoland/rev/7d02cc6ea1e3
Workaround a SQLite query optimization bug. r=daisuke

Comment 9

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/7d02cc6ea1e3

Comment 10

[Marco Bonardo [:mak] (Away 10-21 August)]

Attachment: Bug 2007091 - Workaround a SQLite query optimization bug.

SQLite is optimizing this expiration query wrongly, when using a
bloom filter and an automatic covering index on moz-icons.root.
In addition we're removing the ORDER BY clause as it's expensive. While
it makes sense to first expire older entries, in this case all the
entries are old, so we can save some power by not ordering them.
Also reduce the LIMIT to 50, for smaller I/O operations.

Original Revision: https://phabricator.services.mozilla.com/D277312

Comment 11

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: perf problem causing cpu usage peak
• Code covered by automated testing: no
• Fix verified in Nightly: no
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: While it would be nice, I fear it requires a db with a specific shape of the data, that QA may have difficulties with
• Risk associated with taking this patch: low
• Explanation of risk level: small SQL change, same results but avoiding an automatic index
• String changes made/needed: no
• Is Android affected?: no

Comment 12

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/38763f881427
https://hg.mozilla.org/releases/mozilla-beta/rev/0f65f6c75b35