Closed Bug 1418443 Opened 7 years ago Closed 7 years ago

Group OnVisit events from places::InsertVisitedURIs runnable

Categories

(Firefox :: Migration, enhancement, P3)

Product:
Firefox
Component:
Migration
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: alexical, Assigned: alexical)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1418443 - Send OnVisit notifications from single runnable
59 bytes, text/x-review-board-request
mak
: review+
Details
Not for review
23.61 KB, patch
Details | Diff | Splinter Review 
I'm sure this isn't a new idea, and I know it's been mentioned that we plan on a new events system for places, but I wanted to use this bug to share my results and get at least a cursory understanding of what our current thoughts are on what direction we want to go with the places events.

As far as I can tell, NotifyVisitObservers takes up almost all of the non-wait time on the main thread during history imports. So I tried two methods for reducing this time.

- Since most of the history observers don't actually care about onVisit, and are only subscribing to be notified of URI removals or other places events, we could simply split out onVisit into its own event. To try this without as much work, I simply removed all history observers that don't do anything with onVisit notifications, and the result was a 41% reduction in active time on the main thread during history import.

- Since we do a certain amount of work for every notification we send, independent of what we actually do with that notification, it makes sense to group the visits into an onManyVisits notification. Doing this, and just going with the extremely simple path of calling the the corresponding onVisit method for each item in the onManyVisits event, results in a 65% reduction in active time on the main thread during history import.

To sum up, 41% reduction of work by splitting out nsINavHistoryObserver, and 65% by using an onManyVisits event rather than onVisit. Thoughts, anyone?
The Places observers system is just horrible, we know from a long time. Bug 1340498 is something we'll investigate in the next months. I don't think it's worth the complication of going another direction in the meanwhile.

(In reply to Doug Thayer [:dthayer] from comment #0)
> - Since we do a certain amount of work for every notification we send,
> independent of what we actually do with that notification, it makes sense to
> group the visits into an onManyVisits notification. Doing this, and just
> going with the extremely simple path of calling the the corresponding
> onVisit method for each item in the onManyVisits event, results in a 65%
> reduction in active time on the main thread during history import.

Yes, we should do this regardless, accumulate info in an array, and notify once all the database inserts have been done.
Would help be welcome on Bug 1340498 or is it something you'd prefer someone with more familiarity to work on?
Flags: needinfo?(mak77)
(In reply to Doug Thayer [:dthayer] from comment #2)
> Would help be welcome on Bug 1340498 or is it something you'd prefer someone
> with more familiarity to work on?

I'd like to discuss a possible API for that in Austin with Standard8, additional brainstorming is welcome.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #1)
> (In reply to Doug Thayer [:dthayer] from comment #0)
> > - Since we do a certain amount of work for every notification we send,
> > independent of what we actually do with that notification, it makes sense to
> > group the visits into an onManyVisits notification. Doing this, and just
> > going with the extremely simple path of calling the the corresponding
> > onVisit method for each item in the onManyVisits event, results in a 65%
> > reduction in active time on the main thread during history import.
> 
> Yes, we should do this regardless, accumulate info in an array, and notify
> once all the database inserts have been done.

I'm realizing I may have misunderstood - does "we should do this regardless" mean that a patch that does this in a simple way without the API changes is welcome in the mean time?
Flags: needinfo?(mak77)
(In reply to Doug Thayer [:dthayer] from comment #4)
> I'm realizing I may have misunderstood - does "we should do this regardless"
> mean that a patch that does this in a simple way without the API changes is
> welcome in the mean time?

I'm sorry, I think I misread your original proposal. I thought you were suggesting to send all the onVisit notifications from a single runnable instead of creating one NotifyVisitObservers runnable per place, that looks like what we are doing here: https://searchfox.org/mozilla-central/rev/919dce54f43356c22d6ff6b81c07ef412b1bf933/toolkit/components/places/History.cpp#1061

Instead you meant to add an actual onManyVisits notification to nsINavHistoryObserver and then from there invoke the local onVisit methods.
I wonder how much time is actually spent in creating and sending the runnables, vs how much time is spent in xoconbnect (to actually invoke onVisit). Fixing the runnables problem doesn't require any API change, but I don't know how much it will save.

The API changes is something we may look into, but it's all work that may become shortly obsolete, and we'd still need to coalesce the runnables for the new notifications, because it seems to make sense, so maybe that's a good first step to do here?
Flags: needinfo?(mak77)
the previous "xoconbnect" was clearly meant to be "XPConnect" :\
(In reply to Marco Bonardo [::mak] from comment #5)
> (In reply to Doug Thayer [:dthayer] from comment #4)
> > I'm realizing I may have misunderstood - does "we should do this regardless"
> > mean that a patch that does this in a simple way without the API changes is
> > welcome in the mean time?
> 
> I'm sorry, I think I misread your original proposal. I thought you were
> suggesting to send all the onVisit notifications from a single runnable
> instead of creating one NotifyVisitObservers runnable per place, that looks
> like what we are doing here:
> https://searchfox.org/mozilla-central/rev/
> 919dce54f43356c22d6ff6b81c07ef412b1bf933/toolkit/components/places/History.
> cpp#1061
> 
> Instead you meant to add an actual onManyVisits notification to
> nsINavHistoryObserver and then from there invoke the local onVisit methods.
> I wonder how much time is actually spent in creating and sending the
> runnables, vs how much time is spent in xoconbnect (to actually invoke
> onVisit). Fixing the runnables problem doesn't require any API change, but I
> don't know how much it will save.
> 
> The API changes is something we may look into, but it's all work that may
> become shortly obsolete, and we'd still need to coalesce the runnables for
> the new notifications, because it seems to make sense, so maybe that's a
> good first step to do here?

Yeah - unfortunately a good chunk of the main thread's time seems to be spent in XPConnect. Here's a profile for reference: https://perfht.ml/2zoNdli

I didn't directly compare one runnable with many onVisit notifications vs. one runnable with one onManyVisits notification, but I do know that removing a bunch of history observers that don't actually do anything with the onVisit notification had a sizeable impact, which suggests that avoiding extra XPConnect work has a strong impact for this.

Lastly, we did run a trial on reference hardware with the onManyVisits notification, and saw a 20% reduction in the FX_MIGRATION_HISTORY_JANK_MS probe. It's hard to say how much 20% really means, since the ResponsivenessMonitor just measures jank during the history import window, and not necessarily jank that the history import directly caused, but regardless it means that we're getting rid of >20% of the jank that history import causes, so for the sake of opening up our automigration options I'd hope to do it even if a good portion of it might have to be redone.
well, many runnables end up flooding the main-thread, and that delays pretty much anything and causes jank.
Moreover, to have a single notification you should still collect the info somewhere, and pass that to a runnable, so the work wouldn't be wasted in any case, imo. Single runnable may be the half milestone.
Comment on attachment 8930748 [details]
Bug 1418443 - Send OnVisit notifications from single runnable

https://reviewboard.mozilla.org/r/201846/#review207146


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`


::: toolkit/components/places/History.cpp:701
(Diff revision 1)
> +    , mHistory(History::GetService())
> +  {
> +    aVisits.SwapElements(mVisits);
> +  }
> +
> +  NS_IMETHOD Run() override

Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment on attachment 8930748 [details]
Bug 1418443 - Send OnVisit notifications from single runnable

https://reviewboard.mozilla.org/r/201846/#review207468

Do you have any perf measurement about this change to share?

::: toolkit/components/places/History.cpp:1152
(Diff revision 1)
>  
>      nsresult rv = transaction.Commit();
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    if (mGroupNotifications) {
> +      nsCOMPtr<nsIRunnable> event = new NotifyManyVisitsObservers(mPlaces);

Is there a reason to not just always do this? is it slower for the single place case?
mGroupNotifications was intended to be used to distinguish the type of notification, it would be more fit to pick between onVisit or onManyVisits for example, inside NotifyManyVisitsObservers.

I'd probably try to replace NotifyVisitObservers with your NotifyManyVisitsObservers. The only problem is replacing the single call in StoreAndNotifyEmbedVisit... maybe you could provide 2 constructors, one for an array, one for a single entry and if/else in Run().
If handling the single entry is moved to an helper method, Run() would just call it in a loop or once.
(In reply to Marco Bonardo [::mak] from comment #12)
> Comment on attachment 8930748 [details]
> Bug 1418443 - Send OnVisit notifications from single runnable
> 
> https://reviewboard.mozilla.org/r/201846/#review207468
> 
> Do you have any perf measurement about this change to share?

It's not as big of a win as with onManyVisits implemented. The Run method with this patch takes up an average of 112ms on my machine compared to the 167 without the patch. With onManyVisits it goes down to 59ms. It's hard to say whether it's truly an improvement or not without onManyVisits, since while it's less time on the main thread overall, it happens all at once, which would probably be more noticeable jank if this were an automigrate. Thoughts?
We could still chunk the notifications to avoid the jank, if it ends up being excessive. How would onManyVisits avoid that, if it just calls onVisit in a loop? It sounds like it would end up being the same.
I'm not completely convinced of the onManyVisits measurement, are you sure to have converted all the consumers who care about onVisit? for example bookmarks catch it and forward to onItemVisited, nsNavHistory result has a special behavior where only the result is a real observer and forwards to each container down the hierarchy, sync and newtab do some work. If not all of them are converted, we are measuring a different thing.

I think I'd start from landing this patch and then build on top of it. Maybe the new notifications could exactly begin providing a visit topic, since the 2 systems will likely coexist while we port the various notifications, we can pick which ones should be ported first.
Attached patch Not for review — — Splinter Review 
(In reply to Marco Bonardo [::mak] from comment #15)
> I'm not completely convinced of the onManyVisits measurement, are you sure
> to have converted all the consumers who care about onVisit? for example
> bookmarks catch it and forward to onItemVisited, nsNavHistory result has a
> special behavior where only the result is a real observer and forwards to
> each container down the hierarchy, sync and newtab do some work. If not all
> of them are converted, we are measuring a different thing.

Yeah I got all of those things. Here's the profile if you're interested: https://perfht.ml/2mTDqO9

I'm attaching the patch just in case you want to take a look to verify. It's a bit rough, and I wasn't sure what the best convention was to pass an array through, so let me know if anything stands out to you.
(In reply to Marco Bonardo [::mak] from comment #15)
> We could still chunk the notifications to avoid the jank, if it ends up
> being excessive. How would onManyVisits avoid that, if it just calls onVisit
> in a loop?

It wouldn't, it just mitigates it by being faster (it seems). Regarding chunking, I suppose I'd have to copy the chunk of the array instead of transferring ownership at that point, yeah? Probably still better anyway.
(In reply to Doug Thayer [:dthayer] from comment #17)
> It wouldn't, it just mitigates it by being faster (it seems). Regarding
> chunking, I suppose I'd have to copy the chunk of the array instead of
> transferring ownership at that point, yeah? Probably still better anyway.

Yes, you couldn't transfer ownership, and the cost of copies may be high.
I'm prone to just land what is in the patch, and evaluate further work in follow-up bugs, so single proposals or issues can be evaluated apart.
I'd not be opposite to replacing onVisit with onVisits globally, especially now that add-ons compat is not an issue, provided it would take an array of visit objects, rather than an array for each property. Whether internally it keeps invoking the existing onVisit (converted from IMETHOD to a simple private method) is not a big deal. On the other side, I'd prefer avoiding having both onVisit and onManyVisits APIs, it would just create more confusion in consumers.

(In reply to Doug Thayer [:dthayer] from comment #16)
> Yeah I got all of those things. Here's the profile if you're interested:
> https://perfht.ml/2mTDqO9

The patch seems to have a good coverage indeed. It's surprising, the cost of xpconnect must be sky high. I wonder if there is space to improve there, it would help the whole product. Maybe, in a separate bug, we could involve someone with more experience on it to investigate, or have specific profile of the xpconnect part. Even a few micro-optimizations, when summed up, could help a lot.
Comment on attachment 8930748 [details]
Bug 1418443 - Send OnVisit notifications from single runnable

https://reviewboard.mozilla.org/r/201846/#review207824

LGTM, please file follow-ups for further improvements.
This far the most promising one seems to be throwing away onVisit and replace it with onVisits.

::: toolkit/components/places/History.cpp:692
(Diff revision 2)
>        return NS_OK;
>      }
>  
>      nsNavHistory* navHistory = nsNavHistory::GetHistoryService();
>      if (!navHistory) {
>        NS_WARNING("Trying to notify about a visit but cannot get the history service!");

s/about a visit/visits/
Attachment #8930748 - Flags: review?(mak77) → review+
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
It could be worth doing the PR_Now() call only once, out of the loop, rather than in NotifyVisit, it has a cost (indeed we have a cached now for history insertions exactly to avoid that), and it's unlikely to change, we can pass now to the method.
Last thing, looks like we spend a lot of time in NotifyVisited. It has an AutoScriptBlocker and in general it looks expensive. That sounds like a very good candidate for optimization. it's also all an internal detail so we could change it more easily and, if I remember correctly, it's not critical for it to happen immediately, we could delay it by a little bit. That's what marks links as visited by changing their color. We could potentially dispatch the same runnable again, and in the second Run() call do the NotifyVisited, to give main-thread some breathing.
We could also change SendNotifyVisited to a more batched message, passing an URIParams[] array...
note, comment 21 is not something I expect to happen here, it's mostly thoughts for follow-ups.
Blocks: 1421701
Blocks: 1421703
Blocks: 1421704
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92f6d93364b6
Send OnVisit notifications from single runnable r=mak
https://hg.mozilla.org/mozilla-central/rev/92f6d93364b6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Depends on: 1423612
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: