Bug 23846 – [GTK]Enable DNS prefetching

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Xan Lopez]

Trivial patch. Brian Dash suggested that we should gather some statistics to see if doing this is a real win and/or outweights the performance penalty.

[Xan Lopez]

Created attachment 27483 [details]
DNS prefetching

[Dan Winship]

You can actually just do:

    SoupAddress* address = soup_address_new(hostname.utf8().data(), SOUP_ADDRESS_ANY_PORT);
    soup_address_resolve_async(address, 0, 0, 0, 0);
    g_object_unref(address);

resolve_async() refs the addr when starting the op, and will unref it at the end, and it deals with a NULL callback.

[Collin Jackson]

I think prefetchDNS gets called a lot with an empty hostname when you move your mouse over non-hyperlink DOM elements:

void Chrome::mouseDidMoveOverElement(const HitTestResult& result, unsigned modifierFlags)
312	{
313	    if (result.innerNode()) {
314	        Document* document = result.innerNode()->document();
315	        if (document && document->isDNSPrefetchEnabled())
316	            prefetchDNS(result.absoluteLinkURL().host());
317	    }

For performance it might make sense to check hostname and see if it's empty or not before forwarding it on to the network stack.

[José Millán Soto]

Created attachment 41108 [details]
Results of some tests used to know if prefetching is worth it.

ODS file containing the results to some tests done to see if DNS prefetching is worth it.

[José Millán Soto]

Created attachment 41109 [details]
DNS prefetch patch

Patch based in previous one (submitted by Xan Lopez), with modifications suggested in comment #2 and comment #3.

[Dan Winship]

Ah, Xan and I were talking about this yesterday.

One tricky bit; SoupAddress no longer caches DNS results itself, so what you are testing here is not "libsoup has to do DNS" vs "libsoup does not have to do DNS", but rather "libsoup has to do DNS and the upstream caching DNS server has to query the net for the answer" vs "libsoup has to do DNS and the upstream caching DNS server already has the answer cached".

SoupSession does cache SoupAddresses, so if you could change prefetchDNS() to resolve the session's SoupAddress for that hostname rather than creating its own, then that would probably improve the results a bit more. There's no way to do that currently though; this would require some new libsoup API.

[Holger Freyther]

Comment on attachment 41109 [details]
DNS prefetch patch


>          Reviewed by Simon Hausmann.
> Index: WebCore/page/Chrome.cpp
> ===================================================================
> --- WebCore/page/Chrome.cpp	(revision 49502)
> +++ WebCore/page/Chrome.cpp	(working copy)
> @@ -311,8 +311,11 @@ void Chrome::mouseDidMoveOverElement(con
>  {
>      if (result.innerNode()) {
>          Document* document = result.innerNode()->document();
> -        if (document && document->isDNSPrefetchEnabled())
> -            prefetchDNS(result.absoluteLinkURL().host());
> +        if (document && document->isDNSPrefetchEnabled()) {
> +            String host = result.absoluteLinkURL().host();
> +            if (!host.isEmpty())
> +                prefetchDNS(host);
> +        }
>      }

If you move the check from prefetchDNS to here you will have to update the existing prefetchDNS implementations too to not make them check the string twice.

[Adam Barth]

Comment on attachment 41109 [details]
DNS prefetch patch

I'm not sure adding the null check to Chrome::mouseDidMoveOverElement is the right thing to do.  In Chrome, these events are queued to a background thread to avoid adding latency to mouseover and to history queries.  The null check and a bunch of other prioritization process is done on the background thread.

[José Millán Soto]

Created attachment 41664 [details]
DNS Prefetch patch

This version of the patch checks that a minute has pased since the last time a host was prefetched before resolving it again, and moves the check if hostname is empty to DNSPrefetch function.
It also includes commented code of the implementation of PrefetchDNS to use if soup_session_prepare_for_uri is implemented (See https://bugzilla.gnome.org/show_bug.cgi?id=598948)

[Eric Seidel (no email)]

Comment on attachment 41664 [details]
DNS Prefetch patch

There are several style problems here.  Did you try running check-webkit-style?

Two here:
+static bool checkPrefetched(const String &hostname) {

Two more here:
+    if (hostname.isEmpty()) return;
+    if (checkPrefetched(hostname)) return;

Here:
+    /*
+        This may work while soup_session_prepare_for_uri is not avaliable
+    */

no wrapping rules in webkit:
+    SoupAddress* address = soup_address_new(hostname.utf8().data(),
+                            SOUP_ADDRESS_ANY_PORT);

[Eric Seidel (no email)]

http://webkit.org/coding/coding-style.html is the style guide.

[José Millán Soto]

Created attachment 41724 [details]
DNS prefetch patch

This patch is like the previous one, but it complies with WebKit coding style.

I'm really sorry for not having checked the style in the previous one :(

[José Millán Soto]

Created attachment 41725 [details]
DNS prefetch patch

DNS prefetch patch

This patch is like the previous one, but it complies with WebKit coding style.

I'm really sorry for not having checked the style in the previous one :(

[Gustavo Noronha (kov)]

Comment on attachment 41725 [details]
DNS prefetch patch

> +static bool checkPrefetched(const String& hostname)
> +{
> +    static HashMap<String, double> lastTimePrefetched;
> +    const double ttl = 60;

I would make this a #define. Any reason why you prefer to use a const variable?

> +    /*
> +    soup_session_prepare_for_uri not yet avaliable:
> +        https://bugzilla.gnome.org/show_bug.cgi?id=598948
> +
> +    String uri = "http://"+hostname;
> +    SoupURI* soupUri = soup_uri_new(uri.utf8().data());
> +    soup_session_prepare_for_uri(ResourceHandle::defaultSession(), soupUri);
> +    soup_uri_free(soupUri);
> +    */

We don't usually keep dead code. I would prefer just mentioning the bug report here.

> +    // This may work while soup_session_prepare_for_uri is not avaliable
> +    SoupAddress* address = soup_address_new(hostname.utf8().data(),
> +        SOUP_ADDRESS_ANY_PORT);

No need to break this line, it is not too long by WebKit standards.

> +    soup_address_resolve_async(address, 0, 0, 0, 0);
> +    g_object_unref(address);

According to my understanding of what was said in #webkit-gtk by danw recently, on newer libsoup versions this would have no effect. Here's the relevant discussion:

Oct 22 14:11:12 <danw>  xan: pre-GResolver, there was a one hour cache underneath SoupAddress, so if you did soup_address_new("google.com"), then that might al
ready be resolved. Post-GResolver, that caching layer is gone. However, SoupSession still caches *specific SoupAddresses*. So if you do your own soup_address_n
ew("google.com"), then you're going to have to go to the network to resolve it. But if you queue a request for "google.com", SoupSessino probably
Oct 22 14:11:12 <danw>   already has an already-resolved SoupAddress to use

We may want to have this for older soup versions, but then again, it may be better to queue a NO-OP request? r- for now, for the above. Thanks for working on this.

[Xan Lopez]

> 
> > +    // This may work while soup_session_prepare_for_uri is not avaliable
> > +    SoupAddress* address = soup_address_new(hostname.utf8().data(),
> > +        SOUP_ADDRESS_ANY_PORT);
> 
> No need to break this line, it is not too long by WebKit standards.
> 
> > +    soup_address_resolve_async(address, 0, 0, 0, 0);
> > +    g_object_unref(address);
> 
> According to my understanding of what was said in #webkit-gtk by danw recently,
> on newer libsoup versions this would have no effect. Here's the relevant
> discussion:
> 
> Oct 22 14:11:12 <danw>  xan: pre-GResolver, there was a one hour cache
> underneath SoupAddress, so if you did soup_address_new("google.com"), then that
> might al
> ready be resolved. Post-GResolver, that caching layer is gone. However,
> SoupSession still caches *specific SoupAddresses*. So if you do your own
> soup_address_n
> ew("google.com"), then you're going to have to go to the network to resolve it.
> But if you queue a request for "google.com", SoupSessino probably
> Oct 22 14:11:12 <danw>   already has an already-resolved SoupAddress to use
> 
> We may want to have this for older soup versions, but then again, it may be
> better to queue a NO-OP request? r- for now, for the above. Thanks for working
> on this.

Well, he *is* going to the network to resolve it, right? He explicitly calls the function to resolve the host :)

That being said, I'd prefer to get whatever code dan thinks is OK to handle this (like the prepare function mentioned in the comment), and then just use it. I don't think this is important enough to land workarounds. Also, same thing for the TTL hashtable, I'd rather have soup handle that automatically if possible, otherwise everyone using that API will probably do something similar.

[José Millán Soto]

(In reply to comment #14)
> (From update of attachment 41725 [details])
> > +static bool checkPrefetched(const String& hostname)
> > +{
> > +    static HashMap<String, double> lastTimePrefetched;
> > +    const double ttl = 60;
> 
> I would make this a #define. Any reason why you prefer to use a const variable?

I did it that way becouse it's suggested in WebKit coding style.

> > +    /*
> > +    soup_session_prepare_for_uri not yet avaliable:
> > +        https://bugzilla.gnome.org/show_bug.cgi?id=598948
> > +
> > +    String uri = "http://"+hostname;
> > +    SoupURI* soupUri = soup_uri_new(uri.utf8().data());
> > +    soup_session_prepare_for_uri(ResourceHandle::defaultSession(), soupUri);
> > +    soup_uri_free(soupUri);
> > +    */
> 
> We don't usually keep dead code. I would prefer just mentioning the bug report
> here.

Then I think the best solution is waiting for soup_session_prepare_for_uri to be commited before changing this file.

[Xan Lopez]

Created attachment 43782 [details]
dnsprefetch.diff

OK, this uses the recently landed soup_session_prepare_from_uri. The TTL hash is gone, since that matter is now handled internally by libsoup. It requires libsoup 2.29.3, so I have updated the requirement accordingly.

[Adam Barth]

Attachment 43782 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/network/soup/DNSSoup.cpp:28:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Done processing WebCore/platform/network/soup/DNSSoup.cpp
Total errors found: 1

[Gustavo Noronha (kov)]

Comment on attachment 43782 [details]
dnsprefetch.diff

>  #include "config.h"
> +#include "CString.h"
>  #include "DNS.h"
> -
> -#include "NotImplemented.h"
> +#include "ResourceHandle.h"

As pointed out by the review queue, the CString.h header should be just before ResourceHandle.h, and there should indeed be a blank line between #include "DNS.h" and the CString include.

>  void prefetchDNS(const String& hostname)
>  {
> -    notImplemented();
> +    String uri = "http://"+hostname;
> +    SoupURI* soupUri = soup_uri_new(uri.utf8().data());
> +    soup_session_prepare_for_uri(ResourceHandle::defaultSession(), soupUri);
> +    soup_uri_free(soupUri);
>  }

How about using GOwnPtr here? r=me with whatever you decide in this regard, and with the style problems fixed.

[Xan Lopez]

I pushed this with the style issues fixed and with a bump in the required libsoup version to 2.29.3, r51689.