Summary: | [Gtk] Provide a WebView knob to disable DNS prefetching | ||
---|---|---|---|
Product: | WebKit | Reporter: | Marco Peereboom <marco> |
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | ap, christian, commit-queue, gustavo, todd, webkit.review.bot |
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
Description
Marco Peereboom
2011-03-08 14:51:12 PST
Can you elaborate on why disabling a feature that exists soley for performance reasons makes "the browser quite a bit snappier"? How does DNS prefetching correlate with enable-private-browsing? It doesn't say. dns-prefetch → dns-prefetching Since: 1.2.8 → Since: 1.1.13 It might be nice to briefly explain what DNS prefetching does as it appears in the dcoumentation now. Created attachment 85155 [details]
Proposed patch #2
Updated the patch with all your suggestions, thanks :-) My non-scientific setup is as follows: * OpenBSD 4.9 * WebKitGTK 1.2.7 * xxxterm browser 1.348 * adsuck 2.1 (DNS server that spoofs NXDOMAIN for unwanted servers) I rigged the browser to display total page load time. Where total page load is defined as time spent between WEBKIT_LOAD_PROVISIONAL and WEBKIT_LOAD_FINISHED. I ran it through a number of websites and got the following results: prefetch 0: page load time: 2.148816 -> http://slashdot.org/ prefetch 1: page load time: 7.705360 -> http://slashdot.org/ prefetch 0: page load time: 0.973012 -> http://news.google.com/ prefetch 1: page load time: 19.757806 -> http://news.google.com/ prefetch 0: page load time: 1.504187 -> http://www.wired.com/ prefetch 1: page load time: 3.965751 -> http://www.wired.com/ prefetch 0: page load time: 1.283165 -> http://www.theregister.co.uk/ prefetch 1: page load time: 1.759814 -> http://www.theregister.co.uk/ prefetch 0: page load time: 2.288612 -> http://www.techreview.com/blog/arxiv/ prefetch 1: page load time: 3.196511 -> http://www.techreview.com/blog/arxiv/ prefetch 0: page load time: 2.149060 -> http://www.cnn.com/ prefetch 1: page load time: 5.169388 -> http://www.cnn.com/ prefetch 0: page load time: 2.192299 -> http://espn.go.com/ prefetch 1: page load time: 7.610725 -> http://espn.go.com/ That is what I mean by "quite a bit snappier". The browser seems to load pages at a linear speed instead of bumping along (by observing the completion bar). This is just the performance aspect for me. I can go into the security issues as well but I think those have been described before in other bugs. It's quite hard to test DNS prefetch impact. Once you access a site once, DNS records are cached on your computer, as well as on your provider's server, so tests aren't repeatable. I believe that Google gets its numbers by enabling features like this for a certain percentage of Chrome users, and watching average page load speed, and their data suggested an improvement. --- WebCore/dom/Document.cpp.orig Fri Sep 10 08:20:33 2010 +++ WebCore/dom/Document.cpp Tue Mar 8 10:16:45 2011 It's really weird to see branch patches that aren't made against any svn.webkit.org code. bool m_tiledBackingStoreEnabled : 1; + bool m_dnsPrefetchingEnabled : 1; Tab here. Since the patch isn't made against current sources (and since it's also not marked for review), style bot won't automatically report problems. + priv->dns_prefetching = g_value_get_boolean(value); + break; More tabs. + g_value_set_boolean(value, priv->dns_prefetching); + break; Ditto. I should also mention that there was a number of follow-up bugs about when DNS prefetch should be enabled after bug 28825. See bug 42500, bug 48813, bug 50953, bug 50996. Created attachment 85211 [details]
diff against svn trunk rev 80650
Does Todd's diff meet all requirements? (In reply to comment #6) > Created an attachment (id=85211) [details] > diff against svn trunk rev 80650 + Reviewed no one yet + + Provide a knob to enable/disable DNS prefetching. + DNS prefetching is enabled by default. + https://bugs.webkit.org/show_bug.cgi?id=28825 I recommend you try prepareChangeLog instead of writing it by hand. And please keep the common style, ie: Reviewed by .... title of the bug URL description of the changes I still think this should really mention that DNS prefetching is not affected private browsing because this is not at all obvious. Especially given the privacy-related concerns in other bugs which suggest that people may assume a relation. Thanks a lot for providing the numbers, I personally think it's a reasonable motivation for the setting. Created attachment 85322 [details]
diff against svn 80718
provides a g_object property to toggle dns prefetching
Attachment 85322 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebview.cpp:3297: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebKit/gtk/webkit/webkitwebview.cpp:3451: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 2 in 4 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 85325 [details]
fixed diff against svn 80718
Provide a knob to enable/disable DNS prefetching.
DNS prefetching is enabled by default. This does not
effect 'private browsing', it is an independent knob.
Attachment 85325 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit/gtk/webkit/webkitwebview.cpp:3297: Tab found; better to use spaces [whitespace/tab] [1]
Source/WebKit/gtk/webkit/webkitwebview.cpp:3451: Tab found; better to use spaces [whitespace/tab] [1]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 85327 [details]
take 2 fixed diff against svn 80718
Provide a knob to enable/disable DNS prefetching.
DNS prefetching is enabled by default. This does not
effect 'private browsing', it is an independent knob.
(In reply to comment #13) > Created an attachment (id=85327) [details] > take 2 fixed diff against svn 80718 + Provide a knob to enable/disable DNS prefetching. + DNS prefetching is enabled by default. This does not + effect 'private browsing', it is an independent knob. + https://bugs.webkit.org/show_bug.cgi?id=28825 I meant to mention private browsing *in the documentation*, not the change log. So that users of the API actually see it. I should've been clearer. The bug link is wrong. Created attachment 85336 [details] moved private browsing mention to docs not changelog .. against svn 80722 Provide a knob to enable/disable DNS prefetching. DNS prefetching is enabled by default. https://bugs.webkit.org/show_bug.cgi?id=55973 http://www.pinkbike.com/news/DNS-Prefetching-implications.html A definite negative side effect of dns prefetching. I don't think that the aforelinked article impacts this patch in any way. From mailing list discussion, it sounds like Gtk may need to have DNS prefetch completely disabled, at least on some platforms, and not a preference. Alexey, I think you are missing the point. The ability to toggle this at will permits us to determine if it is indeed good or bad in a given situation. Senslessly hardcoding things is why this patch was generated in the first place, to give the user and/or application the choice, not hardcoding webkit. This is a user preference on both windows and mac, where the majority of the webkit usage remains in terms of browsers on systems. Please let gtk follow in the same footsteps and let us have a knob to make the choice as well. > Please let gtk follow in the same footsteps and let us have a knob to make the choice as well.
It's not true that this is a user preference in Safari. It is a WebKit preference indeed, but it's not exposed to users in Safari UI.
Adding a method in API to toggle something that you know is always bad is a rather strange thing to do. API methods are a permanent liability - you can't take it out once you add it.
But anyway, I'm not stopping anyone here. I'm certainly not in position for making API decisions for Gtk.
Is there anything else missing in this patch? I agree with Alexey that this should not be exposed (at least in Epiphany) to the user, but having a webkit preference makes sense to me, and though I still feel that it makes more sense as a build-time option, I'm not opposed to having it as a runtime option. It should be renamed enable-dns-prefetch to match the convention used in other similar preferences, though. In WebKitGTK+ we adopt a policy to have at least two GTK+ reviewers agree to a new API addition before commiting to it. You got my agreement, provided that we rename it, so if Martin Robinson or Xan agree, we can land this =). Created attachment 85474 [details] diff against 80853 with enable- prefix added as requested Provide a knob to enable/disable DNS prefetching. DNS prefetching is enabled by default. https://bugs.webkit.org/show_bug.cgi?id=55973 Comment on attachment 85474 [details]
diff against 80853 with enable- prefix added as requested
OK, looking good now. Martin expressed he's ok with this on the mailing list and xan said 'why not', so let's do it!
w00t! thanks everybody! Comment on attachment 85474 [details] diff against 80853 with enable- prefix added as requested Clearing flags on attachment: 85474 Committed r80856: <http://trac.webkit.org/changeset/80856> All reviewed patches have been landed. Closing bug. |