1414892 - moz_places_afterinsert_trigger responsible for ~50% of history import times in large profiles

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

[Alex Thayer [:alexical] (she/her)]

I've been profiling and experimenting with importing large Chrome profiles, and I've found that replacing the moz_places_afterinsert_trigger trigger with work outside of SQL (using a hashtable of hosts and then iterating through it once all the inserts have finished), yields around a 50% performance improvement on my machine. I'd like to propose that we scrap this trigger and replace it where we do moz_places inserts with equivalent non-SQL code.

Marco, does this seem suspicious, or raise any red flags to you? I have a proof of concept built out, but since this is unfamiliar territory to me I can't be too confident in my results.

Comment 1

[Marco Bonardo [:mak]]

It's likely. We already know those triggers are slow, due to the moz_hosts updating, also CREATE_UPDATEHOSTS_AFTERDELETE_TRIGGER is slow. Unfortunately Sqlite doesn't have PER STATEMENT triggers, so it runs the trigger once per removed url.
To be clear, this runs off the main-thread, so the change may improve the clock time, should not have interesting effects on jank.

We have bug 843357 tracking a schema optimization to simplify (and speed up) those update queries, we are also planning to stop using "typed" for autofill in bug 1239708, that is also expensive to keep up-to-date. So we were tracking work to improve the situation, but didn't have a chance to look into it yet.

The reason to use triggers here is mostly for data coherence, additionally in the delete case we want to be sure hosts go away for obvious privacy reasons.
If we take the debit care, nothing prevents us from doing a single update instead of using a trigger. Though, we must ensure to do that in all the points where we could insert into moz_places, that includes history, bookmarks, and keywords.
And we must be sure there's no code in the middle that could prevent the moz_hosts update from happening (due to exceptions or such) or we'd generate a coherency problem. It should happen inside the same transaction adding/removing the moz_places entry.

Comment 2

[Marco Bonardo [:mak]]

We should not even iterate each host and fire a query per each, we could probably run a single query with an IN clause.

Comment 3

[Alex Thayer [:alexical] (she/her)]

(In reply to Marco Bonardo [::mak] from comment #2)
> We should not even iterate each host and fire a query per each, we could
> probably run a single query with an IN clause.

The problem with the IN clause is we have to do the fixup_url / get_unreversed_host translations between host and rev_host in the query, which seem to eat up a lot of time[1]. Inlining the fixup_url and get_unreversed_host functions' code directly, and once per host, seems to be a large part of the speed-up I'm seeing.

> To be clear, this runs off the main-thread, so the change may improve the
> clock time, should not have interesting effects on jank.

On a one or two core machine during startup (common conditions for automigrate), I would expect that time spent even on a background thread has a significant effect on the perceived performance of the browser, no? (Provided that time is CPU-heavy, which it seems to be.)

[1]: Why don't we bother marking any of our SQL functions as SQLITE_DETERMINISTIC? It doesn't seem to have a huge effect in this case, but have we played around with that for other cases?

Comment 4

[Marco Bonardo [:mak]]

I have an alternative idea, that we already used successfully in the past and may help, that basically is reimplementing per statement triggers through a temp table.
We can make the current triggers write changes to a temp table (it's in memory, so quick), then issue a DELETE from temp table. a DELETE trigger on the temp table takes care of updates that now are one per host.
This has the advantage of removing most Storage querying overhead, and won't lose any instance. if it happens in the same transaction as the history change, should also be pretty safe.

(In reply to Doug Thayer [:dthayer] from comment #3)
> [1]: Why don't we bother marking any of our SQL functions as
> SQLITE_DETERMINISTIC? It doesn't seem to have a huge effect in this case,
> but have we played around with that for other cases?

no perf win, and we don't use the APIs requiring those, so it would be a mere exercise.

Comment 5

[Matthew N. [:MattN]]

Assigning as P3 for now since it's not a regression.

Comment 6

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1414892 - Optimize moz_places_afterinsert_trigger

This uses a similar strategy as that employed by moz_places_afterdelete_trigger,
creating a temp table which we write host inserts into, and then deleting all
the rows from it when we're done inserting, effectively resulting in a per-
statement trigger to only do the significant work per host.

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

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8926488 [details]
Bug 1414892 - Optimize moz_places_afterinsert_trigger

https://reviewboard.mozilla.org/r/197742/#review203712

The patch looks good to me, I have just a couple minor suggestions.

My only doubt is how much we care to have this in 58 pondering the risk of landing it this late, vs having it in early 59. Is the win you measured previously (50%) still valid? Is the project you are working on to improve migration something tracked for 58?

::: toolkit/components/places/PlacesUtils.jsm:2260
(Diff revision 1)
> +            await db.executeCached("DELETE FROM moz_updatehostsinsert_temp");
> +          })
>            await db.executeCached(
>              `INSERT INTO moz_keywords (keyword, place_id, post_data)
>               VALUES (:keyword, (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url), :post_data)
>              `, { url: url.href, keyword, post_data: postData });

Please also include this insert into the transaction.

::: toolkit/components/places/nsPlacesTriggers.h:120
(Diff revision 1)
> -// trigger will then take care of updating the moz_hosts table.
> +// DELETE trigger will then take care of updating the moz_hosts table.
>  // Note this way we lose atomicity, crashing between the 2 queries may break the
>  // hosts table coherency. So it's better to run those DELETE queries in a single
>  // transaction.
>  // Regardless, this is still better than hanging the browser for several minutes
>  // on a fast machine.

Let's move the comment above CREATE_PLACES_AFTERINSERT_TRIGGER NS_LITERAL_CSTRING and make it a bit more generic like "The next triggers implement a hack to workaround...", so we can have a single comment explaining the process for all the involved triggers.

And then before each trigger just have a brief comment (max couple lines) about it.

Comment 8

[Alex Thayer [:alexical] (she/her)]

(In reply to Marco Bonardo [::mak] from comment #7)
> My only doubt is how much we care to have this in 58 pondering the risk of
> landing it this late, vs having it in early 59. Is the win you measured
> previously (50%) still valid? Is the project you are working on to improve
> migration something tracked for 58?

It was hovering at 40-50% on my machine when I measured with this patch. That said I don't think is all that important until we want to switch to some kind of background migrator (automigrate or something fancier), because based on the one analysis I've seen, migration time doesn't seem to affect retention very much for wizard-based migrations. However, ddurst may have different thoughts?

Comment 9

[David Durst [:ddurst]]

I'd like to test it on reference hardware for manual and automigration.

If it improves things substantially (definition is admittedly subjective), then we might want to push this to 58. But if this is really only empirically beneficial to automigration, then it should be part of the onboarding & activity stream decisions.

Comment 10

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8926488 [details]
Bug 1414892 - Optimize moz_places_afterinsert_trigger

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

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8926488 [details]
Bug 1414892 - Optimize moz_places_afterinsert_trigger

https://reviewboard.mozilla.org/r/197742/#review204470

It looks good, and we are early in thye cycle, let's do it!
Thank you for fixing this, it may help a lot those multiple inserts cases.

::: toolkit/components/places/nsPlacesTriggers.h:98
(Diff revision 2)
>        "SELECT round(avg(substr(url,1,11) = 'http://www.')) FROM moz_places h " \
>        "WHERE (" HOST_TO_REVHOST_PREDICATE ") AND +h.typed = 1 " \
>      ") THEN 'www.' " \
>    "END "
>  
> -/**
> +// The next few triggers are a workaround the lack of FOR EACH STATEMENT in

"a workaround FOR the lack of"

::: toolkit/components/places/nsPlacesTriggers.h:144
(Diff revision 2)
> +    "SELECT " \
> +        "(SELECT id FROM moz_hosts WHERE host = OLD.host), " \
> +        "OLD.host, " \
> +        "MAX(IFNULL((SELECT frecency FROM moz_hosts WHERE host = OLD.host), -1), " \
> +          "(SELECT MAX(frecency) FROM moz_places h " \
> +          "WHERE (" OLDHOST_TO_REVHOST_PREDICATE "))), " \

nit: indent the where once more

::: toolkit/components/places/nsPlacesTriggers.h:147
(Diff revision 2)
> +        "MAX(IFNULL((SELECT frecency FROM moz_hosts WHERE host = OLD.host), -1), " \
> +          "(SELECT MAX(frecency) FROM moz_places h " \
> +          "WHERE (" OLDHOST_TO_REVHOST_PREDICATE "))), " \
> +        "MAX(IFNULL((SELECT typed FROM moz_hosts WHERE host = OLD.host), 0), " \
> +          "(SELECT MAX(typed) FROM moz_places h " \
> +          "WHERE (" OLDHOST_TO_REVHOST_PREDICATE "))), " \

ditto

::: toolkit/components/places/nsPlacesTriggers.h:151
(Diff revision 2)
> +          "(SELECT MAX(typed) FROM moz_places h " \
> +          "WHERE (" OLDHOST_TO_REVHOST_PREDICATE "))), " \
> +        "(" HOSTS_PREFIX_PRIORITY_FRAGMENT \
> +         "FROM ( " \
> +            "SELECT OLD.host AS host " \
> +          ") AS match " \

I'm honestly not sure why we put this AS match here, doesn't look like anything is using it... Could you please try to remove it and check?

Comment 12

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8926488 [details]
Bug 1414892 - Optimize moz_places_afterinsert_trigger

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

Comment 13

[Alex Thayer [:alexical] (she/her)]

Try run looks clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=61a1824b35626e8a4c5c1ed692740670efa7e4a5&selectedJob=144780141

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d02718fccf0
Optimize moz_places_afterinsert_trigger r=mak

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4d02718fccf0

Comment 16

[maggus.staab]

(In reply to Marco Bonardo [::mak] from comment #1)
> It's likely. We already know those triggers are slow, due to the moz_hosts
> updating, also CREATE_UPDATEHOSTS_AFTERDELETE_TRIGGER is slow. Unfortunately
> Sqlite doesn't have PER STATEMENT triggers, so it runs the trigger once per
> removed url.
> To be clear, this runs off the main-thread, so the change may improve the
> clock time, should not have interesting effects on jank.

couldn't you use multi-row inserts, so trigger run only once per "batch-insert"?
see https://stackoverflow.com/questions/16055566/insert-multiple-rows-in-sqlite#16055724

Comment 17

[Marco Bonardo [:mak]]

Yes, we could, but then we should also pay attention to not go over the sql text limit and very large queries tend to be inefficient. Though, it's surely a thing to take into account in the future, thank you for the suggestion.

Comment 18

[Marco Bonardo [:mak]]

I'm also not sure a multi-row insert would hit the trigger only once, since the trigger is FOR EACH ROW, so it may be pointless.