Closed Bug 1341097 Opened 7 years ago Closed 7 years ago

Reduce the number of notifications to the main thread done through UpdatePlaces to improve UI responsiveness for imports

Categories

(Firefox :: Migration, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox52 --- wontfix
firefox53 + verified
firefox54 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

I would like to keep bug 1332225 open and still have something concrete to track these changes, so I'm filing a separate bug.

I'll put patches up in a second. The TL;DR is that a big part of why history imports from Chrome (and to a lesser degree, other browsers) are hanging the UI is that calling the relevant API (UpdatePlaces) does most of its updates per-visit-per-place *and* then sends a number of notifications to the main thread for each of those. Specifically, it sends:

1) a notification that a visit to that URL has happened
2) a notification that the place (ie URL)'s frecency has changed
3) a notification that the place's title has changed (if its title changed OR if it's a new place/URL that we didn't know about, the latter of which is very likely for history imports, certainly at the start of the profile's lifetime ie first startup)
4) a notification to any callbacks passed to the UpdatePlaces API for every individual result / error it sees.

The patches I attach:

a) remove (2) and replace it with a single notification after all places have updated (this is behind an additional flag passed to UpdatePlaces)
b) removes (3) for new places (which fixes bug 1280601)
c) removes (4) based on opt-in bools on the observers callers pass in

Then there's 1 extra small patch that tweaks two minor things I happened to notice on the way.

The overall impact of this is that import of my very large, synthetically created chrome history file (~24000 unique URLs) now hangs the UI 80% less than it did before as measured through Dão's work in bug 1338522. As a side-effect, the import is 8% faster (speeding up the import itself wasn't my primary aim, and might be tricky to balance with these notifications if those are what's hanging the UI - if the import is faster, more of those notifications will hit the main thread in the same timeframe).

Unfortunately it is not possible to remove/replace (1) without potentially breaking consumers, so I haven't attempted that in these patches.
Note for part 3: I haven't extensively reviewed other onVisit() handlers for needing titlechanged-related additions (yet). Most of the ones I've seen in the past have been empty.
Attachment #8839496 - Attachment is obsolete: true
Comment on attachment 8839643 [details]
Bug 1341097 - part 3b: fix tests that expect titlechanged notifications for first visits inserted,

https://reviewboard.mozilla.org/r/114212/#review115688

Thanks! This LGTM, Sync doesn't care about history title changes.
Attachment #8839643 - Flags: review?(kit) → review+
Blocks: 1340498
Comment on attachment 8839231 [details]
Bug 1341097 - part 1: group frecency notifications from history notifications,

https://reviewboard.mozilla.org/r/113930/#review115956

::: toolkit/components/places/History.cpp:880
(Diff revision 2)
> +    MOZ_ASSERT(NS_IsMainThread(), "This should be called on the main thread");
> +    nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
> +    MOZ_ASSERT(navHistory, "Could not get nsNavHistory?!");
> +    if (!navHistory) {
> +      return NS_ERROR_FAILURE;
> +    }

in this case I'd also be fine with NS_ENSURE_STATE. this is unexpected so a warning would be fine and welcome. Otherwise you may NS_WARN_IF. I'm not a big fan of the new longer notation.
Attachment #8839231 - Flags: review?(mak77) → review+
Comment on attachment 8839231 [details]
Bug 1341097 - part 1: group frecency notifications from history notifications,

https://reviewboard.mozilla.org/r/113930/#review115984

::: toolkit/components/places/mozIAsyncHistory.idl:180
(Diff revision 2)
>     *         - Providing an invalid transitionType for a mozIVisitInfo.
>     */
>    [implicit_jscontext]
>    void updatePlaces(in jsval aPlaceInfo,
> -                    [optional] in mozIVisitInfoCallback aCallback);
> +                    [optional] in mozIVisitInfoCallback aCallback,
> +                    [optional] in bool aGroupNotifications);

I forgot, it would be nice to have an additional subtest in test_async_history_api.js
Comment on attachment 8839232 [details]
Bug 1341097 - part 2: allow turning off notifications for individual inserted results when calling updatePlaces,

https://reviewboard.mozilla.org/r/113932/#review115980

please, also update insertMany or file a follow-up to update it, to make onError and onResult optional and set the appropriate flags if they are not passed into.
This looks good, apart that I don't like updateCount as an attribute.

::: toolkit/components/places/History.cpp:930
(Diff revision 3)
>      nsMainThreadPtrHandle<mozIVisitInfoCallback>
>        callback(new nsMainThreadPtrHolder<mozIVisitInfoCallback>(aCallback));
> +    bool ignoreErrors = false, ignoreResults = false;
> +    if (aCallback) {
> +      aCallback->GetIgnoreErrors(&ignoreErrors);
> +      aCallback->GetIgnoreResults(&ignoreResults);

Better to be explicit about the fact we are ignoring errors here, since an old implementation may not have these methods and then these would return an error.
Just use "Unused << "

::: toolkit/components/places/History.cpp:964
(Diff revision 3)
> +    }
> +    return rv;
> +  }
> +
> +  nsresult InnerRun(uint32_t* successfulUpdatedCount) {
> +    *successfulUpdatedCount = 0;

wouldn't it be simpler to have a count property, than to pass around a pointer?

::: toolkit/components/places/History.cpp:3026
(Diff revision 3)
> -  // is dispatched to the background thread first and redirected to the
> +    // is dispatched to the background thread first and redirected to the
> -  // main thread from there to make sure that all database notifications
> +    // main thread from there to make sure that all database notifications
> -  // and all embed or canAddURI notifications have finished.
> +    // and all embed or canAddURI notifications have finished.
> -  if (aCallback) {
> +
> +    // Note: if we're inserting anything, it's the responsibility of
> +    // InsertVisitedURIs to call the completion callback.

please add the reason, looks like it's because here we don't know how many items have been inserted, isn't it?

::: toolkit/components/places/mozIAsyncHistory.idl:114
(Diff revision 3)
> +  readonly attribute bool ignoreErrors;
> +
> +  /**
> +   * Set to the count of successfully updated places
> +   */
> +  attribute unsigned long updatedCount;

what's the reason to make this an attribute, rather than passing it out as an handleCompletion argument?

This is basically the only thing I don't like!

::: toolkit/components/places/tests/unit/test_async_history_api.js:1208
(Diff revision 3)
> +});
> +
> +
>  function run_test() {
>    run_next_test();
>  }

while here, this can be removed.
Attachment #8839232 - Flags: review?(mak77)
Comment on attachment 8839234 [details]
Bug 1341097 - part 4: misc. small optimizations,

https://reviewboard.mozilla.org/r/113936/#review115996
Attachment #8839234 - Flags: review?(mak77) → review+
Comment on attachment 8839643 [details]
Bug 1341097 - part 3b: fix tests that expect titlechanged notifications for first visits inserted,

https://reviewboard.mozilla.org/r/114212/#review116000

::: addon-sdk/source/lib/sdk/places/events.js:127
(Diff revision 1)
> +historyObserver.onVisit = function(url, visitId, time, sessionId,
> +                                   referringId, transitionType, guid, hidden,
> +                                   visitCount, typed, lastKnownTitle) {
> +  // If this is the first visit we're adding, fire title-changed
> +  // in case anyone cares.
> +  if (visitCount <= 1) {

can visitCount be 0? That would sound like a bug?
(In reply to Marco Bonardo [::mak] from comment #20)
> Comment on attachment 8839643 [details]
> Bug 1341097 - part 3b: fix tests that expect titlechanged notifications for
> first visits inserted,
> 
> https://reviewboard.mozilla.org/r/114212/#review116000
> 
> ::: addon-sdk/source/lib/sdk/places/events.js:127
> (Diff revision 1)
> > +historyObserver.onVisit = function(url, visitId, time, sessionId,
> > +                                   referringId, transitionType, guid, hidden,
> > +                                   visitCount, typed, lastKnownTitle) {
> > +  // If this is the first visit we're adding, fire title-changed
> > +  // in case anyone cares.
> > +  if (visitCount <= 1) {
> 
> can visitCount be 0? That would sound like a bug?

It would be, I think, but I figured I would write this defensively... I did the same in the C++ code in the history observer, I think.
Blocks: 1280601
(In reply to :Gijs from comment #21)
> > can visitCount be 0? That would sound like a bug?
> 
> It would be, I think, but I figured I would write this defensively... I did
> the same in the C++ code in the history observer, I think.

what about a MOZ_ASSERT(count > 0) before the notify onVisit instead? :)
Comment on attachment 8839643 [details]
Bug 1341097 - part 3b: fix tests that expect titlechanged notifications for first visits inserted,

https://reviewboard.mozilla.org/r/114212/#review116316

Regardless, this is fine. If we could MOZ_ASSERT before sending onVisit, it would be nicer than overprotecting on the receiver site.
Attachment #8839643 - Flags: review?(mak77) → review+
Comment on attachment 8839233 [details]
Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications for new history entries,

https://reviewboard.mozilla.org/r/113934/#review116342

please also add an xpcshell test (even a subtest of an existing history test) that checks the new onVisit argument is passed, as I will suggest later, it would be nice to have a simple mochitest browser test for the docshell insertions, and an xpcshell test for the code insertions.

What about these 2 consumers:
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/browser/components/newtab/PlacesProvider.jsm#87
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/toolkit/modules/NewTabUtils.jsm#781

Should we fire onLinkChanged from onVisit if visitCount is 1? I think those are basically updating the newtab page thumb title. The only things that currently can add visits apart from browsing are Sync and automigration. For the automigration case this would get the manyFrecenciesChanged thing and likely just reload the whole result (at least I suppose) with the new titles. But I suppose Sync could want the page to appear with a proper title in case it should appear in the new tab page?

::: toolkit/components/places/nsINavHistoryService.idl:675
(Diff revision 3)
>     * @param aTyped
>     *        Whether the URI has been typed or not.
>     *        TODO (Bug 1271801): This will become a count, rather than a boolean.
>     *        For future compatibility, always compare it with "> 0".
> +   * @param aLastKnownTitle
> +   *        The last known title of the page. Might not be from the current visit.

We should add that it can be null, and in such a case the title wasn't known at the time of the notification. That differs from an empty string.

And it would be REALLY nice to have a test for that. The visits that are notified from the docshell should always get null here. That means the test needs a docshell :/
But maybe it's possible to not pass a title to updatePlaces and it may do? I don't recall honestly.
Attachment #8839233 - Flags: review?(mak77)
Attachment #8839643 - Attachment is obsolete: true
Comment on attachment 8839231 [details]
Bug 1341097 - part 1: group frecency notifications from history notifications,

https://reviewboard.mozilla.org/r/113930/#review115984

> I forgot, it would be nice to have an additional subtest in test_async_history_api.js

I added a test.
Comment on attachment 8839232 [details]
Bug 1341097 - part 2: allow turning off notifications for individual inserted results when calling updatePlaces,

https://reviewboard.mozilla.org/r/113932/#review115980

> while here, this can be removed.

Removed in the earlier cset as I was adding a test there anyway.
Comment on attachment 8839233 [details]
Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications for new history entries,

https://reviewboard.mozilla.org/r/113934/#review116342

> We should add that it can be null, and in such a case the title wasn't known at the time of the notification. That differs from an empty string.
> 
> And it would be REALLY nice to have a test for that. The visits that are notified from the docshell should always get null here. That means the test needs a docshell :/
> But maybe it's possible to not pass a title to updatePlaces and it may do? I don't recall honestly.

I updated the comment and added tests to xpcshell as well as a browser test.
(In reply to Marco Bonardo [::mak] from comment #24)
> Comment on attachment 8839233 [details]
> Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications
> for new history entries,
> 
> https://reviewboard.mozilla.org/r/113934/#review116342
> 
> please also add an xpcshell test (even a subtest of an existing history
> test) that checks the new onVisit argument is passed, as I will suggest
> later, it would be nice to have a simple mochitest browser test for the
> docshell insertions, and an xpcshell test for the code insertions.
> 
> What about these 2 consumers:
> http://searchfox.org/mozilla-central/rev/
> 39e4b25a076c59d2e7820297d62319f167871449/browser/components/newtab/
> PlacesProvider.jsm#87
> http://searchfox.org/mozilla-central/rev/
> 39e4b25a076c59d2e7820297d62319f167871449/toolkit/modules/NewTabUtils.jsm#781
> 
> Should we fire onLinkChanged from onVisit if visitCount is 1? I think those
> are basically updating the newtab page thumb title. The only things that
> currently can add visits apart from browsing are Sync and automigration. For
> the automigration case this would get the manyFrecenciesChanged thing and
> likely just reload the whole result (at least I suppose) with the new
> titles. But I suppose Sync could want the page to appear with a proper title
> in case it should appear in the new tab page?

As discussed on IRC, I added code to the new tab page to handle initial onVisit notifications. I need to check what the impact on runtime and hanginess is. I will try to find time to do this tonight or tomorrow morning, but in the worst case this may have to wait until Monday. Logically it shouldn't be any worse than the current state, though.
Comment on attachment 8839232 [details]
Bug 1341097 - part 2: allow turning off notifications for individual inserted results when calling updatePlaces,

https://reviewboard.mozilla.org/r/113932/#review116710

Please, remember to file that bug to use these properties in insert/insertMany, we're not in an extreme hurry so it could even be a mentored bug.
Attachment #8839232 - Flags: review?(mak77) → review+
Comment on attachment 8839233 [details]
Bug 1341097 - part 3: don't dispatch oodles of titlechanged notifications for new history entries,

https://reviewboard.mozilla.org/r/113934/#review116714

::: toolkit/components/places/tests/unit/test_async_history_api.js:104
(Diff revision 4)
>                      aReferringId,
>                      aTransitionType,
> -                    aGUID) {
> -    do_print("onVisit(" + aURI.spec + ", " + aVisitId + ", " + aTime +
> -             ", " + aSessionId + ", " + aReferringId + ", " +
> -             aTransitionType + ", " + aGUID + ")");
> +                    aGUID,
> +                    aHidden,
> +                    aVisitCount, 
> +                    aTyped, 

some trailing spaces here
Attachment #8839233 - Flags: review?(mak77) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6e9c98f1d38f
part 1: group frecency notifications from history notifications, r=mak
https://hg.mozilla.org/integration/autoland/rev/9b5e829e6416
part 2: allow turning off notifications for individual inserted results when calling updatePlaces, r=mak
https://hg.mozilla.org/integration/autoland/rev/0ad0959be648
part 3: don't dispatch oodles of titlechanged notifications for new history entries, r=mak
https://hg.mozilla.org/integration/autoland/rev/9bec0be69329
part 4: misc. small optimizations, r=mak
Ni me to file a bug for updating insertMany.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1343182
Attached patch Patch for 52 (beta/release) (obsolete) — Splinter Review
Approval Request Comment *** CONTAINS CSETS 1-2 ONLY ***
[Feature/Bug causing the regression]: This bug is relevant for automatic as well as manual migration from other browsers
[User impact if declined]: migration of history (especially from Chrome) is much slower and causes a hang-y experience on the main thread, which it shouldn't.
[Is this code covered by automated tests?]: yes!
[Has the fix been verified in Nightly?]: locally, yes, but not by QE. There are also automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: I'm not sure it "needs" a manual test. As this is a perf fix, it's a bit tricky to verify manually, especially after the changes from bug 1340115. The gist of it would be:

1. use a chrome/IE/Edge/Safari profile with a lot of history (10s of 1000s of URLs)
2. manually import history using the option in the library
3. in about:telemetry, check the numbers reported for FX_MIGRATION_HISTORY_JANK_MS before/after this patch . In extreme cases, you might also be able to notice a difference in responsiveness 'by eye'.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not very, inasmuch as any change in the rc part of the beta cycle is "not very" risky
[Why is the change risky/not risky?]:
- it's reasonably well-covered by automated tests
- all the API changes to places are opt-in and only used from the migration code, so any risk is contained to migration of history from other browsers (whether manual or automatic)
- the main change is that we dispatch fewer notifications to the consumer, and the patches update the only affected consumers. As a result, these changes should be safe. I'm explicitly not asking for beta/release uplift of patches 3 and 4 which affect other consumers as well.

[String changes made/needed]: none
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8841931 - Flags: review+
Attachment #8841931 - Flags: approval-mozilla-release?
Attachment #8841931 - Flags: approval-mozilla-beta?
Comment on attachment 8839231 [details]
Bug 1341097 - part 1: group frecency notifications from history notifications,

Approval Request Comment *** FOR PATCHES 1-4 TO AURORA ONLY ***
[Feature/Bug causing the regression]: This bug is relevant for automatic as well as manual migration from other browsers
[User impact if declined]: migration of history (especially from Chrome) is much slower and causes a hang-y experience on the main thread, which it shouldn't.
[Is this code covered by automated tests?]: yes!
[Has the fix been verified in Nightly?]: locally, yes, but not by QE. There are also automated tests.
[Needs manual test from QE? If yes, steps to reproduce]: I'm not sure it "needs" a manual test. As this is a perf fix, it's a bit tricky to verify manually, especially after the changes from bug 1340115. The gist of it would be:

1. use a chrome/IE/Edge/Safari profile with a lot of history (10s of 1000s of URLs)
2. manually import history using the option in the library
3. in about:telemetry, check the numbers reported for FX_MIGRATION_HISTORY_JANK_MS before/after this patch . In extreme cases, you might also be able to notice a difference in responsiveness 'by eye'.

[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not too risky
[Why is the change risky/not risky?]:
- it's reasonably well-covered by automated tests
- there's still a long runway until release to notice any potential problems
- impact is mostly (but, for patches 3-4, not entirely) limited to opt-in from consumers, which are updated as part of the patch

[String changes made/needed]: none
Attachment #8839231 - Flags: approval-mozilla-aurora?
Comment on attachment 8841931 [details] [diff] [review]
Patch for 52 (beta/release)

rc week is too late for perf patches, especially as this isn't a recent regression
Attachment #8841931 - Flags: approval-mozilla-release?
Attachment #8841931 - Flags: approval-mozilla-release-
Attachment #8841931 - Flags: approval-mozilla-beta?
Attachment #8841931 - Flags: approval-mozilla-beta-
Depends on: 1343850
Attachment #8841931 - Attachment is obsolete: true
Note: when uplifting this, we should include the 1-line "gijs was stupid" fix from bug 1343850... I mis-named a local reference to an argument, and when I fixed it when checking this bug in I fixed it in 1 of the duplicate places observers for new tab, but not the other (so bug 1343850 fixes the other). And no, I don't know why we have 2 identical new tab places observer things :-\
Comment on attachment 8839231 [details]
Bug 1341097 - part 1: group frecency notifications from history notifications,

Perf improvement for profile migration. Let's try it in aurora before the merge.
Attachment #8839231 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Andrei, can your team test this in early beta? You might be able to get a copy of Gijs's test Chrome profile. Thanks!
Flags: needinfo?(andrei.vaida)
I tested this on Windows 10 x64 using 54.0a1 (2017-02-27), 53.0b1 build1 (20170307064827) and 54.0a2 (2017-03-07). These are the FX_MIGRATION_HISTORY_JANK_MS results:
- 54.0a1 (2017-02-27)
chrome
1 samples, average = 8988, sum = 8988

2962 |  0  0%
5406 |#########################  1  100%
9867 |  0  0%

- 53.0b1 build1 (20170307064827)
chrome
1 samples, average = 1336, sum = 1336

 487 |  0  0%
 889 |#########################  1  100%
1623 |  0  0%

- 54.0a2 (2017-03-07)
chrome
1 samples, average = 3437, sum = 3437

1623 |  0  0%
2962 |#########################  1  100%
5406 |  0  0%

I used a Chrome profile with about 600 URLs. 
Can you please help me interpret this results? And also, if you consider is necessary, you can provide a copy of your test Chrome profile, in order to re-evaluate the migration process.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Iulia Cristescu, QA [:IuliaC] from comment #49)
> I tested this on Windows 10 x64 using 54.0a1 (2017-02-27), 53.0b1 build1
> (20170307064827) and 54.0a2 (2017-03-07). These are the
> FX_MIGRATION_HISTORY_JANK_MS results:
> - 54.0a1 (2017-02-27)
> chrome
> 1 samples, average = 8988, sum = 8988
> 
> 2962 |  0  0%
> 5406 |#########################  1  100%
> 9867 |  0  0%
> 
> - 53.0b1 build1 (20170307064827)
> chrome
> 1 samples, average = 1336, sum = 1336
> 
>  487 |  0  0%
>  889 |#########################  1  100%
> 1623 |  0  0%
> 
> - 54.0a2 (2017-03-07)
> chrome
> 1 samples, average = 3437, sum = 3437
> 
> 1623 |  0  0%
> 2962 |#########################  1  100%
> 5406 |  0  0%
> 
> I used a Chrome profile with about 600 URLs. 
> Can you please help me interpret this results? And also, if you consider is
> necessary, you can provide a copy of your test Chrome profile, in order to
> re-evaluate the migration process.

Interpretation at this level is straightforward - lower numbers are better. So the march builds (1336, 3437) have (significantly) lower numbers than the nightly from before this fix (8988), which means the fix here is working.

That said, I don't have a straightforward explanation for the (significant) difference between the numbers on 53 and 54. Is this from the same Chrome profile, and with identical/similar/empty Firefox profiles ? If so, I think it warrants opening a followup bug. For that bug, if you could provide me with the Chrome profile in question (either on the bug or privately via email if it's a personal profile) that would be helpful (specifically, the files starting with "History" in the Chrome profile - it should be in %LOCALAPPDATA%/Google/Chrome/User Data/<name-of-profile> . The name of the profile is probably "Default" if you don't have multiple Chrome profiles set up on that machine).
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(iulia.cristescu)
(In reply to :Gijs from comment #50)
>Is this from the same Chrome
> profile, and with identical/similar/empty Firefox profiles ? 
Yes, I used the same Chrome profile and a new Firefox profile for each tested build.

> If so, I think
> it warrants opening a followup bug. For that bug, if you could provide me
> with the Chrome profile in question (either on the bug or privately via
> email if it's a personal profile) that would be helpful (specifically, the
> files starting with "History" in the Chrome profile - it should be in
> %LOCALAPPDATA%/Google/Chrome/User Data/<name-of-profile> . The name of the
> profile is probably "Default" if you don't have multiple Chrome profiles set
> up on that machine).
I will provide the requested information privately, via email.
Flags: needinfo?(iulia.cristescu)
Depends on: 1346796
You need to log in before you can comment on or make changes to this bug.