1421703 - Investigate apparent XPConnect cost in calling nsNavHistory observers with onVisit

Status: RESOLVED FIXED

(Firefox :: Migration, enhancement, P3)

Description

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

When we call all the nsNavHistory observers with the onVisit event from the NotifyManyVisitsObservers runnable, we seem to spend a large amount of time in XPConnect. We've seen that we can reduce this by replacing onVisit with onVisits, but it would be prudent to understand why this cost is so high in the first place before just working around it.

Comment 1

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

Quick note: replacing the lots-of-array-parameters approach with an nsIArray of XPCOM interfaces kills the performance gains here. I don't know if there's a standard way of passing an array of structs through XPConnect, and I can't find any good examples to steal, other than the lots-of-array-parameters approach which does perform quite well.

Comment 2

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

Marco - is there someone who you think would have good insights here? The best that I can tell is that one NativeArray2JS call for a long array is faster than many calls to NativeData2JS, which isn't very mysterious, since I'm sure there's lots of optimizations you can make when working in bulk with many objects of the same type. In other words, there's nothing that sticks out to me in the profiles regarding XPConnect, but maybe someone else can see something. The only thing that did stick out is that I think we can use GetCachedEntries instead of GetEntries from the nsCategoryCache in nsPlacesMacros.h. We spend 11ms on my machine in nsCategoryCache<nsINavHistoryObserver>::AddEntries, and we don't need to do so. It becomes a non-issue for the case I'm trying to optimize if we just change to a batch interface (onManyVisits/onVisits), but it's probably still worth doing.

I'm attaching two profiles that I've been working from, for reference:

w/o onManyVisits: https://perfht.ml/2B0dire
w/ onManyVisits: https://perfht.ml/2AYjQqh

Comment 3

[Marco Bonardo [:mak]]

(In reply to Doug Thayer [:dthayer] from comment #1)
> Quick note: replacing the lots-of-array-parameters approach with an nsIArray
> of XPCOM interfaces kills the performance gains here. I don't know if
> there's a standard way of passing an array of structs through XPConnect.

Maybe :nfroyd knows who could help us understanding if there's space for improvements in nsIArray and if there's space for improvements in repeated xpconnect calls in general.


(In reply to Doug Thayer [:dthayer] from comment #2)
> The only thing that did stick out is that I think we can
> use GetCachedEntries instead of GetEntries from the nsCategoryCache in
> nsPlacesMacros.h. We spend 11ms on my machine in
> nsCategoryCache<nsINavHistoryObserver>::AddEntries, and we don't need to do
> so.

SGTM!

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Marco Bonardo [::mak] (Away 16 Dec - 2 Jan) from comment #3)
> (In reply to Doug Thayer [:dthayer] from comment #1)
> > Quick note: replacing the lots-of-array-parameters approach with an nsIArray
> > of XPCOM interfaces kills the performance gains here. I don't know if
> > there's a standard way of passing an array of structs through XPConnect.
> 
> Maybe :nfroyd knows who could help us understanding if there's space for
> improvements in nsIArray and if there's space for improvements in repeated
> xpconnect calls in general.

nsIArray isn't going to get any smaller.  What did the patch to use nsIArray look like?

Comment 5

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

(In reply to Nathan Froyd [:froydnj] (doing reviews while in Austin) from comment #4)
> nsIArray isn't going to get any smaller.  What did the patch to use nsIArray
> look like?

Unfortunately I didn't have the forethought to upload it anywhere, so it's on my home desktop. However, it was essentially this:

- Add a new nsIVisitData interface which has attributes for all of the parameters of the onVisit notification [1]
- Add an nsVisitData class implementing said interface
- When we would be calling the onVisit observers in a loop, instead append a new nsVisitData to an nsIArray
- Outside the loop, call a new onManyVisits method which accepts said nsIArray as an argument
- Inside the observers of onManyVisits, for loop over said nsIArray using QueryElementAt, and call the previous onVisit method on the observer directly

The patch had a slight performance degradation, as I recall, whereas the patch which did the same thing using many parameters which were each an array of primitives (using [array, size_is(...)]) had significant performance gains. Is this the right way to be using an nsIArray, or was I doing something suboptimal?


[1]: https://searchfox.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#657

Comment 6

[Nathan Froyd [:froydnj]]

nsIArray.QueryElementAt is horrendously inefficient compared to nsTArray<SomethingMoreConcreteThannsISupports>, as it will QueryInterface for every element access.

The [array, size_is(...)] approach will let you avoid casting to nsISupports and then QueryInterface'ing back down, which is probably part of your performance improvement.  You could have interface methods like:

  unsigned long numThings();
  nsIThing getThing(unsigned long index);

you'll still be doing virtual calls in your inner loop, which will be inefficient.  I think I'd have to see the patch to say more.

Do we just think the [array, size_is(...)] approach is inelegant, or is there some other reason for avoiding it?

Comment 7

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

(In reply to Nathan Froyd [:froydnj] (doing reviews while in Austin) from comment #6)
> Do we just think the [array, size_is(...)] approach is inelegant, or is
> there some other reason for avoiding it?

I'm not sure. In my original patch I chose the [array, size_is(...)] option for speculative performance reasons and because I saw a few other places in tree where people were using it. Marco, what were your reasons for preferring an array of visit objects rather than [array, size_is(...)]. I know you want to keep the API clean but I don't want to put words in your mouth.

Comment 8

[Marco Bonardo [:mak]]

(In reply to Doug Thayer [:dthayer] from comment #7)
> Marco, what were your reasons for
> preferring an array of visit objects rather than [array, size_is(...)]. I
> know you want to keep the API clean but I don't want to put words in your
> mouth.

It sounds a little bit crazy that we have to pass N arrays of primitive properties, instead of a single array of well-defined objects, thus, if we could have a similar win with a cleaner API (single Array of N objects) I would prefer it. XPConnect should provide something for that :(
I don't understand though if Nathan is suggesting using an [array] of objects or multiple [array]s of primitives, my original comment was only about using multiple arrays of primitives.
And yes, it's mostly about inelegant API, so if there's no way out, we will proceed with what we can do.

Comment 9

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

I'll give an [array] of nsIVisitData a shot, and see how it performs. It's definitely going to perform worse because of the virtual calls, but the cost may not be significant enough to care.

Comment 10

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

Update to any interested parties: I have a patch all ready for this, which is now green on try, but it will have to wait until after the break. We'll have to talk then about whether we want to land it this late in the cycle or if we should wait for the trains to leave.

Comment 11

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

Attachment: Bug 1421703 - replace onVisit with onVisits

There's a heavy enough overhead to going through XPConnect for
every observer for every visit on the nsINavHistoryObserver
interface, so this patch reduces that by replacing the single-
visit notification with one which accepts an array of visits.

Some notes: To avoid problems with the orderings of the various
ways in which we notify about visits, we have to send our bulk
onVisits notification before doing any of the others. This does
mean it technically behaves slightly different than the prior
approach of interleaving the notifications, but I can't find any
way in which this has any consequences to the end result, and it
doesn't break any tests.

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

Comment 12

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

Attachment: Bug 1421703 - Fix tests to use onVisits

Fairly straightforward. Fixes the tests that were broken due to
the changes introduced in part 1.

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

Comment 13

[Code Review Bot [:reviewbot]]

Comment on attachment 8939577 [details]
Bug 1421703 - replace onVisit with onVisits

https://reviewboard.mozilla.org/r/208232/#review215484


C/C++ static analysis found 1 defect in this patch.

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

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


::: toolkit/components/places/History.cpp:265
(Diff revision 1)
> +    aLastKnownTitle.Assign(mLastKnownTitle);
> +    return NS_OK;
> +  }
> +
> +private:
> +  virtual ~nsVisitData() {};

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]

  virtual ~nsVisitData() {};
          ^
                         = default;

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 8939577 [details]
Bug 1421703 - replace onVisit with onVisits

https://reviewboard.mozilla.org/r/208232/#review216684

It's mostly fine, there are a couple things to cleanup yet, and a couple possible follow-ups (unless you are willing to fix those here).

::: browser/components/extensions/ext-history.js:117
(Diff revision 1)
> -          url: uri.spec,
> +            url: uri.spec,
> -          title: lastKnownTitle || "",
> +            title: lastKnownTitle || "",
> -          lastVisitTime: time / 1000,  // time from Places is microseconds,
> +            lastVisitTime: time / 1000,  // time from Places is microseconds,
> -          visitCount,
> +            visitCount,
> -          typedCount: typed,
> +            typedCount: typed,
> -        };
> +          };

I understand the will to preserve blame, but this does a lot of temp assignments that are not really useful, I'd prefer to directly use visit.* properties directly in the data object.

::: browser/extensions/activity-stream/lib/PlacesFeed.jsm:75
(Diff revision 1)
>    }
>  
>    // Empty functions to make xpconnect happy
>    onBeginUpdateBatch() {}
>    onEndUpdateBatch() {}
>    onVisit() {}

Shouldn't onVisit be removed?

::: browser/modules/WindowsPreviewPerTab.jsm:846
(Diff revision 1)
>    },
>  
>    /* nsINavHistoryObserver implementation */
>    onBeginUpdateBatch() {},
>    onEndUpdateBatch() {},
>    onVisit() {},

ditto

::: services/sync/modules/engines/history.js:453
(Diff revision 1)
>      this.onDeleteAffectsGUID(uri, guid, reason, "onDeleteURI", SCORE_INCREMENT_XLARGE);
>    },
>  
> -  onVisit(uri, vid, time, session, referrer, trans, guid) {
> +  onVisits(aVisits) {
>      if (this.ignoreAll) {
> -      this._log.trace("ignoreAll: ignoring visit for " + guid);
> +      this._log.trace("ignoreAll: ignoring visits - length = " + aVisits.length);

can we rather log the list of guids being ignored, as it was before? It may be useful for debugging purposes

::: services/sync/modules/engines/history.js:457
(Diff revision 1)
>      if (this.ignoreAll) {
> -      this._log.trace("ignoreAll: ignoring visit for " + guid);
> +      this._log.trace("ignoreAll: ignoring visits - length = " + aVisits.length);
>        return;
>      }
> -
> +    for (let {uri, guid} of aVisits) {
> -    this._log.trace("onVisit: " + uri.spec);
> +      this._log.trace("onVisit: " + uri.spec);

s/onVisit/onVisits/ (let's keep the naming consistent for now).

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm:1033
(Diff revision 1)
>    },
>  
>    onTitleChanged() {},
>    onBeginUpdateBatch() {},
>    onEndUpdateBatch() {},
>    onVisit() {},

please remove old onVisit

::: toolkit/components/places/History.cpp:198
(Diff revision 1)
> +    , mHidden(aHidden)
> +    , mVisitCount(aVisitCount)
> +    , mTyped(aTyped)
> +    , mLastKnownTitle(aLastKnownTitle)
> +  {
> +  }

please MOZ_ASSERT that this is always constructed on the mainthread. Maybe worth even checking in the destructor. nsIURI is not thread-safe, as well as this whole object.

::: toolkit/components/places/History.cpp:841
(Diff revision 1)
> +    if (places.Length() > 0) {
> +      navHistory->NotifyOnVisits(places.Elements(), places.Length());
> +    }
> +
>      PRTime now = PR_Now();
>      if (mPlaces.Length() > 0) {

I wonder if it's really worth the code complication of having both mPlace and mPlaces paths VS the cost of allocating a single-item array in any case. Do you recall if your measurements shown an interesting benefit here, or are we just overzealous on this micro-optimization?
This is not something to do here, but may be worth a code cleanup follow-up if the benefit is nonexistent.

::: toolkit/components/places/nsINavHistoryService.idl:27
(Diff revision 1)
>  interface nsINavHistoryQueryOptions;
>  interface nsINavHistoryResult;
>  interface nsINavHistoryBatchCallback;
>  
> +/**
> + * This interface was created specifically for passing visit information

s/was created/exists/

::: toolkit/components/places/nsNavBookmarks.cpp:3210
(Diff revision 1)
> +    MOZ_ALWAYS_SUCCEEDS(place->GetVisitCount(&visitCount));
> +    uint32_t typed;
> +    MOZ_ALWAYS_SUCCEEDS(place->GetTyped(&typed));
> +    nsString lastKnownTitle;
> +    MOZ_ALWAYS_SUCCEEDS(place->GetLastKnownTitle(lastKnownTitle));
> +    nsresult rv = OnVisit(uri, visitId, time, 0, referrerId, transitionType, guid, hidden,

could we instead build an ItemVisitData object and pass that one ot OnVisit? or just merge the two... This doesn't seem really readable and we only need part of the data.

::: toolkit/components/places/nsNavHistoryResult.h:100
(Diff revision 1)
> -                     int64_t aSessionId, int64_t aReferringId,          \
> -                     uint32_t aTransitionType, const nsACString& aGUID, \
> -                     bool aHidden, uint32_t aVisitCount,                \
> -                     uint32_t aTyped, const nsAString& aLastKnownTitle) \
> -                     __VA_ARGS__;
> +                   int64_t aSessionId, int64_t aReferringId,            \
> +                   uint32_t aTransitionType, const nsACString& aGUID,   \
> +                   bool aHidden, uint32_t aVisitCount,                  \
> +                   uint32_t aTyped, const nsAString& aLastKnownTitle);  \
> +  NS_IMETHOD OnVisits(nsIVisitData** aVisits,                           \
> +                      uint32_t aVisitsCount) __VA_ARGS__;

Maybe we could simplify this, the scope of NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL and NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL was to have onVisit differ when onVisit was part of nsINavHistoryObserver. Thus we couldn't just use NS_DECL_NSINAVHISTORYOBSERVER.
But now we could partially do that.

NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL would just be
NS_DECL_NSINAVBOOKMARKOBSERVER
NS_DECL_NSINAVHISTORYOBSERVER
+ onVisit
It could even be hardcoded directly in nsNavHistoryResult class definition.

The same way NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL could be hardcoded directly in nsNavHistoryQueryResultNode. NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE would become useless, nsNavHistoryQueryResultNode would define what is currently inside it, it would not have onVisits, but have onVisit.

This would avoid some of the back and forth reading the current code requires, since everything would be defined at its place.
Though, since this is a little bit OT, I'd also be fine with a follow-up bug for it, if it makes sense to you.

::: toolkit/components/places/nsNavHistoryResult.cpp:4741
(Diff revision 1)
> +    MOZ_ALWAYS_SUCCEEDS(place->GetVisitCount(&visitCount));
> +    uint32_t typed;
> +    MOZ_ALWAYS_SUCCEEDS(place->GetTyped(&typed));
> +    nsString lastKnownTitle;
> +    MOZ_ALWAYS_SUCCEEDS(place->GetLastKnownTitle(lastKnownTitle));
> +    nsresult rv = OnVisit(uri, visitId, time, 0, referrerId, transitionType,

If onVisit is not using all of these, we can simplify its signature, indeed now it's no more a copy of an idl interface, it's just an helper method.

::: toolkit/components/places/nsPlacesExpiration.js:539
(Diff revision 1)
>    onClearHistory: function PEX_onClearHistory() {
>      // History status is clean after a clear history.
>      this.status = STATUS.CLEAN;
>    },
>  
>    onVisit() {},

remove onVisit

::: toolkit/components/thumbnails/PageThumbs.jsm:835
(Diff revision 1)
>    },
>  
>    onTitleChanged() {},
>    onBeginUpdateBatch() {},
>    onEndUpdateBatch() {},
>    onVisit() {},

ditto

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 8939578 [details]
Bug 1421703 - Fix tests to use onVisits

https://reviewboard.mozilla.org/r/208922/#review216718

nothing blocking, just a few nits, but let's have a second look at the whole thing

::: browser/components/migration/tests/unit/test_automigration.js:622
(Diff revision 1)
>    let observer = {
>      onBeginUpdateBatch() {},
>      onEndUpdateBatch() {},
>      onVisit(uri) {
>        wrongMethodDeferred.reject(new Error("Unexpected call to onVisit " + uri.spec));
>      },

remove this?

::: docshell/test/browser/browser_bug420605.js:58
(Diff revision 1)
>  
>      /* Global history observer that triggers for the two test URLs above. */
>      var historyObserver = {
>          onBeginUpdateBatch: function() {},
>          onEndUpdateBatch: function() {},
> -        onVisit: function(aURI, aVisitID, aTime, aSessionId, aReferringId,
> +        onVisits: function(aVisits) {},

omit the argument

::: docshell/test/browser/browser_bug503832.js:18
(Diff revision 1)
>      let fragmentPromise = new Promise(resolve => {
>          /* Global history observer that triggers for the two test URLs above. */
>          var historyObserver = {
>              onBeginUpdateBatch: function() {},
>              onEndUpdateBatch: function() {},
> -            onVisit: function(aURI, aVisitID, aTime, aSessionId, aReferringId,
> +            onVisits: function(aVisits) {},

ditto

::: services/sync/tests/unit/head_helpers.js:582
(Diff revision 1)
>          resolve();
>        }
>      }
>      let observer = {
> -      onVisit(uri) {
> -        done("added", uri);
> +      onVisits(visits) {
> +        equal(visits.length, 1);

We prefer to always have the Assert prefix, so Assert.equal.
We try to be consistent in xpcshell, browser-chrome still has a mixture of the 2 styles, even if new tests tend to specify Assert.

::: toolkit/components/places/tests/browser/browser_bug399606.js:27
(Diff revision 1)
>        info("Received onVisit: " + aURI.spec);
>        if (aURI.equals(this.expectedURI)) {
>          this.count++;
>        }
>      },
> +    onVisits(aVisits) {

Since onVisit is short, please merge the methods

::: toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js:80
(Diff revision 1)
>    // PERM_REDIRECT_VISIT_BONUS.
>    redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS;
>    let redirectNotified = false;
>  
> -  let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => {
> -    info("Received onVisit: " + uri.spec);
> +  let visitedPromise = PlacesTestUtils.waitForNotification("onVisits", visits => {
> +    is(visits.length, 1, "Was notified for the right number of visits.");

ditto

::: toolkit/components/places/tests/browser/browser_multi_redirect_frecency.js:113
(Diff revision 1)
>    // PERM_REDIRECT_VISIT_BONUS.
>    redirectTargetFrecency += PERM_REDIRECT_VISIT_BONUS;
>    let redirectNotified = false;
>  
> -  let visitedPromise = PlacesTestUtils.waitForNotification("onVisit", uri => {
> -    info("Received onVisit: " + uri.spec);
> +  let visitedPromise = PlacesTestUtils.waitForNotification("onVisits", visits => {
> +    is(visits.length, 1, "Was notified for the right number of visits.");

ditto

::: toolkit/components/places/tests/browser/browser_settitle.js:28
(Diff revision 1)
>    let titleChangedPromise = new Promise(resolve => {
>      var historyObserver = {
>        data: [],
>        onBeginUpdateBatch() {},
>        onEndUpdateBatch() {},
> -      onVisit(aURI, aVisitID, aTime, aSessionID, aReferringID,
> +      onVisits(aVisits) {},

omit the argument

::: toolkit/components/places/tests/history/test_async_history_api.js:96
(Diff revision 1)
>    this.callback = aCallback;
>  }
>  VisitObserver.prototype = {
>    __proto__: NavHistoryObserver.prototype,
> -  onVisit(aURI,
> -                    aVisitId,
> +  onVisits(aVisits) {
> +    do_print("onVisits()!!!");

do_print has been replaced by info(), look out for bitrotting or introducing new usage

::: toolkit/components/places/tests/history/test_async_history_api.js:114
(Diff revision 1)
> +    } = aVisits[0];
> +    let args = [
> +      visitId, time, referrerId, transitionType, guid,
> +      hidden, visitCount, typed, lastKnownTitle,
> +    ];
> +    do_print("onVisit(" + uri.spec + args.join(", ") + ")");

ditto

::: toolkit/components/places/tests/queries/test_history_queries_titles_liveUpdate.js:189
(Diff revision 1)
>      let uri = NetUtil.newURI(data.uri);
>      let origTitle = data.title;
>      data.title = "match";
> +
> +    dump_table("moz_places");
> +    dump_table("moz_historyvisits");

leftover?

::: toolkit/components/places/tests/queries/test_history_queries_titles_liveUpdate.js:196
(Diff revision 1)
> +    do_print("Adding " + uri.spec);
>      await PlacesTestUtils.addVisits({ uri, title: data.title,
>                                        visitDate: data.lastVisit });
> +
> +    dump_table("moz_places");
> +    dump_table("moz_historyvisits");

more

::: toolkit/components/places/tests/queries/test_history_queries_titles_liveUpdate.js:205
(Diff revision 1)
> +    do_print("Clobbering " + uri.spec);
>      await PlacesTestUtils.addVisits({ uri, title: data.title,
>                                        visitDate: data.lastVisit });
> +
> +    dump_table("moz_places");
> +    dump_table("moz_historyvisits");

more

::: toolkit/components/places/tests/unit/test_history_observer.js:75
(Diff revision 1)
> +    Assert.equal(referrerId, 0);
> +    Assert.equal(transitionType, TRANSITION_TYPED);
> +    do_check_guid_for_uri(uri, guid);
> +    Assert.ok(!hidden);
> +    Assert.equal(visitCount, 1);
> +    Assert.equal(typed, 1);

nit: this is not preserving blame, so fwiw you could even just do let visit = visits[0] and use visit.prop in the Asserts

::: toolkit/components/places/tests/unit/test_history_observer.js:105
(Diff revision 1)
> +    Assert.equal(referrerId, 0);
> +    Assert.equal(transitionType, TRANSITION_FRAMED_LINK);
> +    do_check_guid_for_uri(uri, guid);
> +    Assert.ok(hidden);
> +    Assert.equal(visitCount, 1);
> +    Assert.equal(typed, 0);

ditto

Comment 16

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

Comment on attachment 8939577 [details]
Bug 1421703 - replace onVisit with onVisits

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

Comment 17

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

Comment on attachment 8939578 [details]
Bug 1421703 - Fix tests to use onVisits

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

Comment 18

[Marco Bonardo [:mak]]

Comment on attachment 8939577 [details]
Bug 1421703 - replace onVisit with onVisits

https://reviewboard.mozilla.org/r/208232/#review217068

::: toolkit/components/places/nsNavHistoryResult.h:81
(Diff revision 2)
>                             const nsACString &aGUID) __VA_ARGS__;        \
>    NS_IMETHOD OnDeleteVisits(nsIURI* aURI, PRTime aVisitTime,            \
>                              const nsACString& aGUID, uint16_t aReason,  \
>                              uint32_t aTransitionType) __VA_ARGS__;
>  
>  // The internal version has an output aAdded parameter, it is incremented by

This comment should be moved before the nsNavHistoryQueryResultNode onVisit method, since it's the one with the aAdded parameter

Rather here for now we could have a comment like "The internal version is used by query nodes."

We will remove these once we can remove the overlapping batching methods in bookmarks...

::: toolkit/components/places/nsNavHistoryResult.h:133
(Diff revision 2)
> +  nsresult OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,
> +                   uint32_t aTransitionType, const nsACString& aGUID,
> +                   bool aHidden, uint32_t aVisitCount,
> +                   const nsAString& aLastKnownTitle);
> +  NS_IMETHOD OnVisits(nsIVisitData** aVisits,                           \
> +                      uint32_t aVisitsCount) override;

please move the IMETHOD just after NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL above

::: toolkit/components/places/nsNavHistoryResult.h:639
(Diff revision 2)
>    bool IsContainersQuery();
>  
>    virtual nsresult OpenContainer() override;
>  
>    NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL
> +  NS_IMETHOD OnVisit(nsIURI* aURI, int64_t aVisitId, PRTime aTime,

nsresult instead of NS_IMETHOD, it's not from the interface

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 8939578 [details]
Bug 1421703 - Fix tests to use onVisits

https://reviewboard.mozilla.org/r/208922/#review217074

::: toolkit/components/places/tests/browser/browser_bug399606.js:23
(Diff revisions 1 - 2)
> -        this.count++;
> -      }
> -    },
>      onVisits(aVisits) {
>        for (let {uri} of aVisits) {
> -        this.onVisit(uri);
> +        info("Received onVisit: " + uri.spec);

"onVisits"

Comment 20

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

Marco, do you feel comfortable with me landing these for 59, or should I wait until the start of the cycle for 60?

Comment 21

[Marco Bonardo [:mak]]

I don't see particular issues here that may require a delay, especially without legacy add-ons around.

Comment 22

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

Comment on attachment 8939577 [details]
Bug 1421703 - replace onVisit with onVisits

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

Comment 23

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

Comment on attachment 8939578 [details]
Bug 1421703 - Fix tests to use onVisits

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

Comment 24

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

Comment on attachment 8939577 [details]
Bug 1421703 - replace onVisit with onVisits

https://reviewboard.mozilla.org/r/208232/#review216684

> Maybe we could simplify this, the scope of NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL and NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL was to have onVisit differ when onVisit was part of nsINavHistoryObserver. Thus we couldn't just use NS_DECL_NSINAVHISTORYOBSERVER.
> But now we could partially do that.
> 
> NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL would just be
> NS_DECL_NSINAVBOOKMARKOBSERVER
> NS_DECL_NSINAVHISTORYOBSERVER
> + onVisit
> It could even be hardcoded directly in nsNavHistoryResult class definition.
> 
> The same way NS_DECL_BOOKMARK_HISTORY_OBSERVER_INTERNAL could be hardcoded directly in nsNavHistoryQueryResultNode. NS_DECL_BOOKMARK_HISTORY_OBSERVER_BASE would become useless, nsNavHistoryQueryResultNode would define what is currently inside it, it would not have onVisits, but have onVisit.
> 
> This would avoid some of the back and forth reading the current code requires, since everything would be defined at its place.
> Though, since this is a little bit OT, I'd also be fine with a follow-up bug for it, if it makes sense to you.

I'm not sure if I'm missing something or not here. AFAICT NS_DECL_NSINAVBOOKMARKOBSERVER and NS_DECL_NSINAVHISTORYOBSERVER collide on onBeginUpdateBatch and onEndUpdateBatch, so changing 
NS_DECL_BOOKMARK_HISTORY_OBSERVER_EXTERNAL to be those + onVisit would not build. No?

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a542159042f5
replace onVisit with onVisits r=mak
https://hg.mozilla.org/integration/autoland/rev/f228865f492c
Fix tests to use onVisits r=mak

Comment 26

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a542159042f5
https://hg.mozilla.org/mozilla-central/rev/f228865f492c

Comment 27

[Ed Lee :Mardak]

(In reply to Marco Bonardo [::mak] from comment #14)
> ::: browser/extensions/activity-stream/lib/PlacesFeed.jsm:75
> >    onVisit() {}
> Shouldn't onVisit be removed?
This didn't end up getting ported to activity stream code base, so it accidentally got switched back to onVisit from onVisits. Should it actually be onVisits or just removed?

Comment 28

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

Oh woops - it should be an empty onVisits() {} to satisfy the interface.

Comment 29

[[github robot]]

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/e51ee0272e93f4821f76ddb05f7d77d661aebb66
chore(mc): Port Bug 1421703 - Investigate apparent XPConnect cost in calling nsNavHistory observers with onVisit