Bug 147824 – NetworkProcess: DNS prefetch happens in the Web Process

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

DNS prefetch requests started in the WebProcess should be sent to the network process when it's enabled.

[Carlos Garcia Campos]

Created attachment 258612 [details]
Patch

[Alexey Proskuryakov]

rdar://problem/12760351

[Alexey Proskuryakov]

Comment on attachment 258612 [details]
Patch

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

Thank you for tackling this, it's good cleanup! I think that this need to be updated to not use strategies though.

> Source/WebCore/html/HTMLAnchorElement.cpp:259
> +                    platformStrategies()->loaderStrategy()->prefetchDNS(document().completeURL(parsedURL).host());

We are gradually getting rid of strategies in favor of client calls. Strategies were needed for the chromium port, and even then, I wasn't quite sure why.

> Source/WebCore/loader/LoaderStrategy.h:31
>  #include <wtf/Vector.h>
> +#include <wtf/text/WTFString.h>

Can probably use wtf/Forward.h here.

[Carlos Garcia Campos]

Created attachment 258713 [details]
Updated patch

Use FrameLoaderClient instead of PlatformStrategies.

[Alexey Proskuryakov]

Comment on attachment 258713 [details]
Updated patch

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

> Source/WebCore/html/HTMLAnchorElement.cpp:257
> -                    prefetchDNS(document().completeURL(parsedURL).host());
> +                    document().frame()->loader().client().prefetchDNS(document().completeURL(parsedURL).host());

I think that the document can have a null frame here, in which case we'll crash. Please add a test case for this, and fix the crash if I'm right.

> Source/WebCore/loader/LinkLoader.cpp:101
> +            document.frame()->loader().client().prefetchDNS(href.host());

Ditto.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
> +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());

Can m_page be null here?

[Carlos Garcia Campos]

Comment on attachment 258713 [details]
Updated patch

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

>> Source/WebCore/html/HTMLAnchorElement.cpp:257
>> +                    document().frame()->loader().client().prefetchDNS(document().completeURL(parsedURL).host());
> 
> I think that the document can have a null frame here, in which case we'll crash. Please add a test case for this, and fix the crash if I'm right.

That's why I added && document().frame() to the previous if condition

>> Source/WebCore/loader/LinkLoader.cpp:101
>> +            document.frame()->loader().client().prefetchDNS(href.host());
> 
> Ditto.

Ditto.

>> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
>> +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());
> 
> Can m_page be null here?

m_page is created in the constructor, and deleted in ::close(), so if we are here we should have a valid page, I can add an assert

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 258713 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258713&action=review
> 
> >> Source/WebCore/html/HTMLAnchorElement.cpp:257
> >> +                    document().frame()->loader().client().prefetchDNS(document().completeURL(parsedURL).host());
> > 
> > I think that the document can have a null frame here, in which case we'll crash. Please add a test case for this, and fix the crash if I'm right.
> 
> That's why I added && document().frame() to the previous if condition
> 
> >> Source/WebCore/loader/LinkLoader.cpp:101
> >> +            document.frame()->loader().client().prefetchDNS(href.host());
> > 
> > Ditto.
> 
> Ditto.
> 
> >> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
> >> +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());
> > 
> > Can m_page be null here?
> 
> m_page is created in the constructor, and deleted in ::close(), so if we are
> here we should have a valid page, I can add an assert

I could also use node->document().frame()->loader().client() checking frame() is not null, since we are already checking node is not null.

[Alexey Proskuryakov]

Comment on attachment 258713 [details]
Updated patch

Oops.

[Carlos Garcia Campos]

Committed r188331: <http://trac.webkit.org/changeset/188331>

[Chris Dumez]

Reopening as I did a speculative roll out of this patch in <http://trac.webkit.org/changeset/190364> to address a ~2.5% PLT regression. I am not 100% sure this was caused by this patch but it seems the most likely culprit in the regression range. I'll take care of re-landing the patch myself if the perf bots do not recover after the roll out.

[Chris Dumez]

(In reply to comment #10)
> Reopening as I did a speculative roll out of this patch in
> <http://trac.webkit.org/changeset/190364> to address a ~2.5% PLT regression.
> I am not 100% sure this was caused by this patch but it seems the most
> likely culprit in the regression range. I'll take care of re-landing the
> patch myself if the perf bots do not recover after the roll out.

The roll out was successful. The bots showed a 2 to 2.5% progression on page load time after the roll out.

[Carlos Garcia Campos]

The patch is quite simple, it just sends a message to the network process to do the prefetch there. So, my guess is that we are sending too many messages. We could probably cache in the web process the hosts already prefetched to avoid sending messages to the network process.

[Chris Dumez]

(In reply to comment #12)
> The patch is quite simple, it just sends a message to the network process to
> do the prefetch there. So, my guess is that we are sending too many
> messages. We could probably cache in the web process the hosts already
> prefetched to avoid sending messages to the network process.

Yes, too much IPC can definitely slow down the PLT. This has happened a few months back due to diagnostic logging. Looking at the patch, one other possibility (I haven't looked deeply into it) is that the new document.frame() checks sometimes fail and we end up not doing DNS prefetches in cases where we used to?

[Carlos Garcia Campos]

Created attachment 262440 [details]
Updated patch

This patch caches the hosts already prefetched.

[Alexey Proskuryakov]

Comment on attachment 262440 [details]
Updated patch

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

> Source/WebKit2/NetworkProcess/NetworkProcess.h:197
> +    HashSet<String> m_dnsPrefetchedHosts;

This HashSet grows indefinitely, and NetworkProcess can exist for weeks or even for months.

> Source/WebKit2/WebProcess/WebProcess.h:368
> +    HashSet<String> m_dnsPrefetchedHosts;

Is it OK to keep hosts here for a long time? I think that this may entirely disable the optimization of prefetching link hosts on hover - the hosts were already prefetched when loading, but the OS resolver may have well invalidated the cache already.

DNS prefetching is a really weird thing. We don't have any way to measure its effectiveness, which is not how we usually go about performance optimizations.

[Carlos Garcia Campos]

(In reply to comment #15)
> Comment on attachment 262440 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262440&action=review
> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.h:197
> > +    HashSet<String> m_dnsPrefetchedHosts;
> 
> This HashSet grows indefinitely, and NetworkProcess can exist for weeks or
> even for months.

I thought about it, my initial idea was to cache only in the web process, since the real performance problem is likely when the same web process requests the same hosts a lot of times, rather than different web processes requesting the same host. So we could simplify it by only caching in the web process and assuming that we might end up requesting the same host by two different web processes.

> > Source/WebKit2/WebProcess/WebProcess.h:368
> > +    HashSet<String> m_dnsPrefetchedHosts;
> 
> Is it OK to keep hosts here for a long time? I think that this may entirely
> disable the optimization of prefetching link hosts on hover - the hosts were
> already prefetched when loading, but the OS resolver may have well
> invalidated the cache already.

Good point. Maybe we should only ensure we don't ask the same hosts in a short period of time, instead. For example when hovering, we start a lot of requests for the same link when the mouse is moving.

> DNS prefetching is a really weird thing. We don't have any way to measure
> its effectiveness, which is not how we usually go about performance
> optimizations.

Yeah, indeed.

[Carlos Garcia Campos]

Created attachment 262588 [details]
Updated patch

Simpler approach, not caching in the network process and clearing the web process cache after a short period of time of no DNS requests. This should avoid asking for the same host more than once when loading a page or hovering a link.

[Carlos Garcia Campos]

Alexey, what do you think about this simpler approach?

[Alexey Proskuryakov]

It makes sense to me overall (I didn't review in detail).

I think that Chris and Gavin would the the right people to decide when it's a good time to land this, so that they can get accurate performance results quickly.

[Chris Dumez]

Comment on attachment 262588 [details]
Updated patch

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

I like the approach in general.

> Source/WebKit2/WebProcess/WebProcess.cpp:1462
> +    if (!usesNetworkProcess()) {

I think it should be factored so that we only to the usesNetworkProcess() is ENABLE(NETWORK_PROCESS) is true.

e.g.
#if ENABLE(NETWORK_PROCESS)
    if (usesNetworkProcess()) {
        // do stuff
        return;
    }
#endif
    WebCore::prefetchDNS(hostname);

> Source/WebKit2/WebProcess/WebProcess.cpp:1473
> +    static const double dnsPrefetchedHostsCleanupDelay = 5;

I believe we prefer the following pattern now:
const auto dnsPrefetchedHostsCleanupDelay = 5_s;

> Source/WebKit2/WebProcess/WebProcess.cpp:1474
> +    m_dnsPrefetchedHostsCleanupTimer.stop();

Using a hysteresis activity, these 2 lines would become a call to impulse().

> Source/WebKit2/WebProcess/WebProcess.h:220
> +    void didPrefetchDNS(const String&);

Where is this implemented?

> Source/WebKit2/WebProcess/WebProcess.h:221
> +    void initializeDNSPrefetchedHosts(const Vector<String>&);

ditto.

> Source/WebKit2/WebProcess/WebProcess.h:370
> +    WebCore::Timer m_dnsPrefetchedHostsCleanupTimer;

Seems like a good use for HysteresisActivity instead of Timer.

[Darin Adler]

Comment on attachment 262588 [details]
Updated patch

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

> Source/WebCore/loader/FrameLoaderClient.h:33
> +#include "DNS.h"

We should keep the implementation of the virtual function out of the header, that way we don’t need to include DNS.h in this header. Generally speaking it’s not so valuable to have implementations of virtual functions in the header rather than a cpp file since it’s not common to be able to expand them in line at the call site.

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
> +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());

Are we guaranteed that m_page is non-null?

[Carlos Garcia Campos]

(In reply to comment #20)
> Comment on attachment 262588 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262588&action=review
> 
> I like the approach in general.
> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:1462
> > +    if (!usesNetworkProcess()) {
> 
> I think it should be factored so that we only to the usesNetworkProcess() is
> ENABLE(NETWORK_PROCESS) is true.
> 
> e.g.
> #if ENABLE(NETWORK_PROCESS)
>     if (usesNetworkProcess()) {
>         // do stuff
>         return;
>     }
> #endif
>     WebCore::prefetchDNS(hostname);

Sure!

> > Source/WebKit2/WebProcess/WebProcess.cpp:1473
> > +    static const double dnsPrefetchedHostsCleanupDelay = 5;
> 
> I believe we prefer the following pattern now:
> const auto dnsPrefetchedHostsCleanupDelay = 5_s;
> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:1474
> > +    m_dnsPrefetchedHostsCleanupTimer.stop();
> 
> Using a hysteresis activity, these 2 lines would become a call to impulse().

Ah, I didn't know HysteresisActivity class, it seems fit well here.

> > Source/WebKit2/WebProcess/WebProcess.h:220
> > +    void didPrefetchDNS(const String&);
> 
> Where is this implemented?

Leftovers from previous iteration.

> > Source/WebKit2/WebProcess/WebProcess.h:221
> > +    void initializeDNSPrefetchedHosts(const Vector<String>&);
> 
> ditto.

Ditto.

> > Source/WebKit2/WebProcess/WebProcess.h:370
> > +    WebCore::Timer m_dnsPrefetchedHostsCleanupTimer;
> 
> Seems like a good use for HysteresisActivity instead of Timer.

Indeed, thanks.

[Carlos Garcia Campos]

(In reply to comment #21)
> Comment on attachment 262588 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=262588&action=review
> 
> > Source/WebCore/loader/FrameLoaderClient.h:33
> > +#include "DNS.h"
> 
> We should keep the implementation of the virtual function out of the header,
> that way we don’t need to include DNS.h in this header. Generally speaking
> it’s not so valuable to have implementations of virtual functions in the
> header rather than a cpp file since it’s not common to be able to expand
> them in line at the call site.

I agree, the advantage in this case that we don't have a common cpp file was to avoid having the same implementation in all FrameLoader clients expect the WebKit2 one. I'll make it pure virtual then, to ensure all clients implement it.

> > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
> > +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());
> 
> Are we guaranteed that m_page is non-null?

Same question ap made in comment #5, which confirms I should add an assert there :-)

[Carlos Garcia Campos]

Created attachment 263456 [details]
Updated patch

Addressed all review comments

[Chris Dumez]

(In reply to comment #23)
> (In reply to comment #21)
> > Comment on attachment 262588 [details]
> > Updated patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=262588&action=review
> > 
> > > Source/WebCore/loader/FrameLoaderClient.h:33
> > > +#include "DNS.h"
> > 
> > We should keep the implementation of the virtual function out of the header,
> > that way we don’t need to include DNS.h in this header. Generally speaking
> > it’s not so valuable to have implementations of virtual functions in the
> > header rather than a cpp file since it’s not common to be able to expand
> > them in line at the call site.
> 
> I agree, the advantage in this case that we don't have a common cpp file was
> to avoid having the same implementation in all FrameLoader clients expect
> the WebKit2 one. I'll make it pure virtual then, to ensure all clients
> implement it.
> 
> > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
> > > +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());
> > 
> > Are we guaranteed that m_page is non-null?
> 
> Same question ap made in comment #5, which confirms I should add an assert
> there :-)

m_page can be null after WebPage::close() is called so I think we should null-check.

[Carlos Garcia Campos]

(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > Comment on attachment 262588 [details]
> > > Updated patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=262588&action=review
> > > 
> > > > Source/WebCore/loader/FrameLoaderClient.h:33
> > > > +#include "DNS.h"
> > > 
> > > We should keep the implementation of the virtual function out of the header,
> > > that way we don’t need to include DNS.h in this header. Generally speaking
> > > it’s not so valuable to have implementations of virtual functions in the
> > > header rather than a cpp file since it’s not common to be able to expand
> > > them in line at the call site.
> > 
> > I agree, the advantage in this case that we don't have a common cpp file was
> > to avoid having the same implementation in all FrameLoader clients expect
> > the WebKit2 one. I'll make it pure virtual then, to ensure all clients
> > implement it.
> > 
> > > > Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:632
> > > > +        m_page->mainFrame().loader().client().prefetchDNS(downcast<Element>(*node).absoluteLinkURL().host());
> > > 
> > > Are we guaranteed that m_page is non-null?
> > 
> > Same question ap made in comment #5, which confirms I should add an assert
> > there :-)
> 
> m_page can be null after WebPage::close() is called so I think we should
> null-check.

Yes, but I guess you can't tap on a closed page. This is a private method called from two WebPage::potentialTapAtPosition() and WebPage::tapHighlightAtPosition(). Both methods are message handlers, that aren't called if the page is closed, and both methods use m_page without any null check before calling sendTapHighlightForNodeIfNecessary().

[Chris Dumez]

Comment on attachment 263456 [details]
Updated patch

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

r=me

> Source/WebKit2/WebProcess/WebProcess.cpp:167
> +    , m_dnsPrefetchedHostsCleanupHysteresis(std::bind(&WebProcess::dnsPrefetchedHostsCleanupHysteresisUpdated, this, std::placeholders::_1))

Could have also been all in one line:
, m_dnsPrefetchHysteresis([this] (HystererisState state) { if (state == HysteresisState::Stopped) m_dnsPrefetchedHosts.clear(); });

> Source/WebKit2/WebProcess/WebProcess.h:368
> +    WebCore::HysteresisActivity m_dnsPrefetchedHostsCleanupHysteresis;

I prefer the naming: m_dnsPrefetchHystereris. This way when the activity stops, it is clearer that what stopped is a DNS prefetching activity.

[Carlos Garcia Campos]

(In reply to comment #27)
> Comment on attachment 263456 [details]
> Updated patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263456&action=review
> 
> r=me

Thanks!

> > Source/WebKit2/WebProcess/WebProcess.cpp:167
> > +    , m_dnsPrefetchedHostsCleanupHysteresis(std::bind(&WebProcess::dnsPrefetchedHostsCleanupHysteresisUpdated, this, std::placeholders::_1))
> 
> Could have also been all in one line:
> , m_dnsPrefetchHysteresis([this] (HystererisState state) { if (state ==
> HysteresisState::Stopped) m_dnsPrefetchedHosts.clear(); });

Yes, I thought about that, but discarded it to avoid the single line if, but being a single statement if it doesn't look that bad, I'll change it.

> > Source/WebKit2/WebProcess/WebProcess.h:368
> > +    WebCore::HysteresisActivity m_dnsPrefetchedHostsCleanupHysteresis;
> 
> I prefer the naming: m_dnsPrefetchHystereris. This way when the activity
> stops, it is clearer that what stopped is a DNS prefetching activity.

Agree

[Carlos Garcia Campos]

Committed r191381: <http://trac.webkit.org/changeset/191381>

[Chris Dumez]

(In reply to comment #29)
> Committed r191381: <http://trac.webkit.org/changeset/191381>

No regression of Mac page load time :)
I'll report on the iOS status later.

[Carlos Garcia Campos]

(In reply to comment #30)
> (In reply to comment #29)
> > Committed r191381: <http://trac.webkit.org/changeset/191381>
> 
> No regression of Mac page load time :)
> I'll report on the iOS status later.

Phew, great, thanks!

[Chris Dumez]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > Committed r191381: <http://trac.webkit.org/changeset/191381>
> > 
> > No regression of Mac page load time :)
> > I'll report on the iOS status later.
> 
> Phew, great, thanks!

No page load time regression on iOS either, congrats :)

[Ryosuke Niwa]

It looks like this patch regressed Dromaeo by 2-4%.

[Alexey Proskuryakov]

> It looks like this patch regressed Dromaeo by 2-4%.

Was that the original, or the latest patch? 

Original: <http://trac.webkit.org/changeset/188331>
Rollout: <http://trac.webkit.org/changeset/190364>
Re-landed: <http://trac.webkit.org/changeset/191381>

If it's the latest, where do we track the regression?

[Ryosuke Niwa]

(In reply to comment #34)
> > It looks like this patch regressed Dromaeo by 2-4%.
> 
> Was that the original, or the latest patch? 
> 
> Original: <http://trac.webkit.org/changeset/188331>
> Rollout: <http://trac.webkit.org/changeset/190364>
> Re-landed: <http://trac.webkit.org/changeset/191381>
> 
> If it's the latest, where do we track the regression?

Sorry, I forgot to comment. Looks like the rollout fixed the regression and re-land didn't cause a new regression.