1421701 - Look into chunking NotifyManyVisitsObservers rather than sending all visits at once

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

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

Bug 1418443 replaced NotifyVisitObservers with NotifyManyVisitsObservers, which reduces overall time taken by the main thread, but results in all of that time being taken at once. If we chunk the runnables into groups of, say, 100 visits, we might get the same reduction in overall time, but not take it all at once, thus reducing jank.

Comment 1

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

Attachment: Bug 1421701 - Chunk visits when notifying main thread

Since processing several hundred visits synchronously on the
main thread can be somewhat expensive, and since the main thread
might be idle while the storage worker is running, it makes sense
to chunk the messages into groups of 100.

We did have a concern that this changes the move of the underlying
array to a copy, which might bring some cost with it. For small
arrays, we simply move the underlying array to avoid the copy to
mitigate it. However, for large arrays I was unable to observe any
significant performance cost anyway, so I think we're in the clear.

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

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 8934310 [details]
Bug 1421701 - Chunk visits when notifying main thread

https://reviewboard.mozilla.org/r/205228/#review213308

::: toolkit/components/places/History.cpp:1092
(Diff revision 1)
>            NS_ENSURE_SUCCESS(rv2, rv2);
>          }
>        }
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> +      int32_t numRemaining = mPlaces.Length() - (i + 1);

can be moved inside the if (shouldChunkNotifications) body

::: toolkit/components/places/History.cpp:1095
(Diff revision 1)
>        NS_ENSURE_SUCCESS(rv, rv);
>  
> +      int32_t numRemaining = mPlaces.Length() - (i + 1);
> +      if (shouldChunkNotifications) {
> +        notificationChunk.AppendElement(place);
> +        if (notificationChunk.Length() > NOTIFY_VISITS_CHUNK_SIZE ||

why ">", it sounds like this will notify 101 entries?

Comment 3

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

Comment on attachment 8934310 [details]
Bug 1421701 - Chunk visits when notifying main thread

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

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8934310 [details]
Bug 1421701 - Chunk visits when notifying main thread

https://reviewboard.mozilla.org/r/205228/#review213438

Thank you!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15a13cdc34d5
Chunk visits when notifying main thread r=mak

Comment 6

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/15a13cdc34d5