Bug 206438 – Frequent sync BackForwardBackListCount/BackForwardForwardListCount IPC on reddit.com

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Frequent sync BackForwardBackListCount/BackForwardForwardListCount IPC on reddit.com. When scrolling on reddit.com, you frequently see 2 consecutive sync IPCs (WebPageProxy::BackForwardBackListCount then WebPageProxy::BackForwardForwardListCount) from the WebContent process to the UIProcess. Those are bad for performance. This happens every time the script on the back accesses history.length, which is unfortunate, since this history length rarely changes.

[Chris Dumez]

Created attachment 388082 [details]
Patch

[Sam Weinig]

Comment on attachment 388082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388082&action=review

> Source/WebKit/ChangeLog:18
> +        2. Cache those counts in WebBackForwardListProxy and blow away the cached counts whenever the back/forward list changes. In the
> +           common case (where the back/forward list rarely changes), we now see a single sync IPC instead of many (verified on reddit.com).

If we are going to start caching, why do we need even one sync ipc? In what cases does the WebProcess not know about items added or removed from the list?

[Darin Adler]

Comment on attachment 388082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388082&action=review

>> Source/WebKit/ChangeLog:18
>> +           common case (where the back/forward list rarely changes), we now see a single sync IPC instead of many (verified on reddit.com).
> 
> If we are going to start caching, why do we need even one sync ipc? In what cases does the WebProcess not know about items added or removed from the list?

In other words, once we start caching we introduce a race, and there is no need to use synchronous IPC to avoid the race. We should just include the counts in the messages that is currently the "invalidate caches" message. Right?

[Chris Dumez]

Comment on attachment 388082 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388082&action=review

>>> Source/WebKit/ChangeLog:18
>>> +           common case (where the back/forward list rarely changes), we now see a single sync IPC instead of many (verified on reddit.com).
>> 
>> If we are going to start caching, why do we need even one sync ipc? In what cases does the WebProcess not know about items added or removed from the list?
> 
> In other words, once we start caching we introduce a race, and there is no need to use synchronous IPC to avoid the race. We should just include the counts in the messages that is currently the "invalidate caches" message. Right?

I am not convinced there is a race here since the changes to the list in the UIProcess are made by IPCs from the WebContent process. I invalidate the local cache, and then send the IPC to the UIProcess that will update its list. I guess we could do what Darin suggests and have IPCs that invalidate the cache also send back the counts. I would need to see how much extra complexity this adds though. It is also trickier than it sounds because not all IPCs that update the list in the UIProcess are synchronous. For example, the WebProcess sends a WebPageProxy::BackForwardAddItem async IPC to the UIProcess, which may change those counts. I don't want to make this IPC synchronous so I cannot get updated counts synchronously.

The WebContent process currently does not maintain a BackForward "list", it merely has a map of items and asks the UIProcess whenever it needs to do something with history. I guess we could maintain more state in the WebContent process in order to avoid the sync IPC altogether but this would likely be significant refactoring. Given that the back/forward list does not change very often, especially when running the page's script, I do believe that my patch gets us most of the perf win, with very little complexity.

[Chris Dumez]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 388082 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388082&action=review
> 
> >>> Source/WebKit/ChangeLog:18
> >>> +           common case (where the back/forward list rarely changes), we now see a single sync IPC instead of many (verified on reddit.com).
> >> 
> >> If we are going to start caching, why do we need even one sync ipc? In what cases does the WebProcess not know about items added or removed from the list?
> > 
> > In other words, once we start caching we introduce a race, and there is no need to use synchronous IPC to avoid the race. We should just include the counts in the messages that is currently the "invalidate caches" message. Right?
> 
> I am not convinced there is a race here since the changes to the list in the
> UIProcess are made by IPCs from the WebContent process. I invalidate the
> local cache, and then send the IPC to the UIProcess that will update its
> list. I guess we could do what Darin suggests and have IPCs that invalidate
> the cache also send back the counts. I would need to see how much extra
> complexity this adds though. It is also trickier than it sounds because not
> all IPCs that update the list in the UIProcess are synchronous. For example,
> the WebProcess sends a WebPageProxy::BackForwardAddItem async IPC to the
> UIProcess, which may change those counts. I don't want to make this IPC
> synchronous so I cannot get updated counts synchronously.
> 
> The WebContent process currently does not maintain a BackForward "list", it
> merely has a map of items and asks the UIProcess whenever it needs to do
> something with history. I guess we could maintain more state in the
> WebContent process in order to avoid the sync IPC altogether but this would
> likely be significant refactoring. Given that the back/forward list does not
> change very often, especially when running the page's script, I do believe
> that my patch gets us most of the perf win, with very little complexity.

I looked into this more and I am still think the patch is correct.
1. I do not think there is a race because the WebBackForwardList changes in the UIProcess are made as a result of IPC messages (e.g. WebPageProxy::BackForwardClear) that are sent by the WebContent process and I make sure the clear the cache in the WebContent process *before* sending those IPCs to the UIProcess.
2. There would be a race if we did what Darin suggest and include counts in response to messages that invalidate the cache. For example, if the WebPageProxy::BackForwardClear async IPC included a response with the updated counts, then responses to WebCore while we're waiting for the IPC response would use stale data. This would not be an issue if IPCs such as WebPageProxy::BackForwardClear were synchronous but we do not want to add more sync IPC.

I do not have a better proposal at the moment. I agree it would be good to get rid of the sync IPC entirely but this patch at least gets us a most of the way there in terms of perf since history rarely changes.

[Chris Dumez]

Created attachment 388435 [details]
Patch

[Alex Christensen]

Comment on attachment 388435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388435&action=review

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:133
> +    return m_page ? cacheListCountsIfNecessary().backCount : 0;

It looks strange to have the m_page check here, where m_page is not used.  Let's move it into cacheListCountsIfNecessary

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60
> +               BuildableName = "libANGLE.a"

This change shouldn't be here.

[Chris Dumez]

Created attachment 388436 [details]
Patch

[Chris Dumez]

Created attachment 388439 [details]
Patch

[Chris Dumez]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 388435 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388435&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:133
> > +    return m_page ? cacheListCountsIfNecessary().backCount : 0;
> 
> It looks strange to have the m_page check here, where m_page is not used. 
> Let's move it into cacheListCountsIfNecessary
> 
> > WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:60
> > +               BuildableName = "libANGLE.a"
> 
> This change shouldn't be here.

Done.

[Chris Dumez]

Comment on attachment 388439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388439&action=review

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:63
> +    clearCachedListCounts();

Technically, when the UIProcess restores the session, it could send the counts along with the sessionState so we could populate here. Let me know if you think we should do this.

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:102
>      m_page->send(Messages::WebPageProxy::BackForwardAddItem(toBackForwardListItemState(item.get())));

We could do a sendWithAsyncReply() here and get the updated counts back. Let me know if you think I should do this.

[Chris Dumez]

ping review?

[Darin Adler]

Comment on attachment 388439 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388439&action=review

>> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:63
>> +    clearCachedListCounts();
> 
> Technically, when the UIProcess restores the session, it could send the counts along with the sessionState so we could populate here. Let me know if you think we should do this.

Our past experience is that anything we can do to avoid sync calls ends up helping us prevent surprise performance hiccups. So it seems we’d want to do anything we could do to cut down on them. But despite the explanations you gave earlier, still honestly not sure I understand why we can’t get rid of the sync messaging entirely, which seems like the best longer term solution.

>> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:102
>>      m_page->send(Messages::WebPageProxy::BackForwardAddItem(toBackForwardListItemState(item.get())));
> 
> We could do a sendWithAsyncReply() here and get the updated counts back. Let me know if you think I should do this.

Same thought.

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:113
> +    m_cachedBackForwardListCounts = WTFMove(backForwardListCounts);

No need to use WTFMove for a structure containing two integers.

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:148
> +        m_cachedBackForwardListCounts = WTFMove(backForwardListCounts);

Ditto.

[Chris Dumez]

(In reply to Darin Adler from comment #13)
> Comment on attachment 388439 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388439&action=review
> 
> >> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:63
> >> +    clearCachedListCounts();
> > 
> > Technically, when the UIProcess restores the session, it could send the counts along with the sessionState so we could populate here. Let me know if you think we should do this.
> 
> Our past experience is that anything we can do to avoid sync calls ends up
> helping us prevent surprise performance hiccups. So it seems we’d want to do
> anything we could do to cut down on them. But despite the explanations you
> gave earlier, still honestly not sure I understand why we can’t get rid of
> the sync messaging entirely, which seems like the best longer term solution.
> 
> >> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:102
> >>      m_page->send(Messages::WebPageProxy::BackForwardAddItem(toBackForwardListItemState(item.get())));
> > 
> > We could do a sendWithAsyncReply() here and get the updated counts back. Let me know if you think I should do this.
> 
> Same thought.

Will follow-up for these optimizations.

> 
> > Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:113
> > +    m_cachedBackForwardListCounts = WTFMove(backForwardListCounts);
> 
> No need to use WTFMove for a structure containing two integers.
> 
> > Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:148
> > +        m_cachedBackForwardListCounts = WTFMove(backForwardListCounts);
> 
> Ditto.

[Chris Dumez]

Created attachment 388818 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388818 [details]
Patch

Clearing flags on attachment: 388818

Committed r255135: <https://trac.webkit.org/changeset/255135>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58908146>