RESOLVED FIXED 45072
PreloadScanner doesn't find image while executing script in head 
https://bugs.webkit.org/show_bug.cgi?id=45072
Summary PreloadScanner doesn't find image while executing script in head
Tony Gentilcore
Reported 2010-09-01 17:16:19 PDT
The PreloadScanner kicks off the download for subsequent images when parsing is blocked by a script in the <body>. This is demonstrated here: http://stevesouders.com/cuzillion/?c0=bj1hfff2_0_f&c1=bi1hfff2_0_f&t=1283385832400 However, it doesn't seem to work when the parsing blocking script is in the <head>. See: http://stevesouders.com/cuzillion/?c0=hj1hfff2_0_f&c1=bi1hfff2_0_f&t=1283385793856 It would be an optimization to fix this.
Attachments
Patch (13.41 KB, patch)
2011-03-29 09:58 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Patch for landing (7.13 KB, patch)
2011-03-31 09:08 PDT, Tony Gentilcore 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Sreeram Ramachandran
Comment 1 2010-09-16 17:48:38 PDT
The PreloadScanner does kick off requests for other things (*) in the <head> when blocked by a script. However, nothing in the <body> is scanned. See: http://stevesouders.com/cuzillion/?c0=hj1hfff2_0_f&c1=hc1hfff2_0_f&c2=bc1hfff2_0_f&t=1284684371 (*) The only "things" that it seems to recognize are other scripts and stylesheets. A background image specified in an inline stylesheet isn't recognized.
Tony Gentilcore
Comment 2 2011-01-28 15:21:59 PST
I'm going to look into this next.
Tony Gentilcore
Comment 3 2011-01-28 16:17:46 PST
It looks like http://trac.webkit.org/changeset/35801 added functionality to prevent scanning past the head in order to improve first paint time. Antti, I've been playing around with http://code.google.com/p/web-page-replay/ to get some reproducible network benchmarks. I started by asking the question of how much the preload scanner helps overall (results: http://gent.ilcore.com/2011/01/webkit-preloadscanner.html). I'm curious if you have any thoughts on this methodology and how you got the numbers you mentioned in the ChangeLog (25% first paint improvement, no change in total load time). It is counterintuitive to me that this would be the case as I typically think of each subresource as dominated by RTT instead of bandwidth. I'm going to try to reproduce on a variety of network settings and see what happens.
Antti Koivisto
Comment 4 2011-01-28 19:22:07 PST
That change was done to improve first display performance on low bandwidth (aka cellular) networks. We really should be adjusting this kind of stuff based on current latency and bandwidth.
Tony Gentilcore
Comment 5 2011-01-31 14:05:12 PST
As a first attempt, I hacked up a build that simply comments out CachedResourceLoader::preload()'s ability to enqueue. Instead it always calls requestPreload(). I compared 44 top sites over a 5Mbps/1Mbps/40ms (up/down/rtt) simulated connection. With the change, the load event is reached on average about 3% sooner, but the first paint event is moved 7% later. The behavior affects different sites in drastically different ways, so the averages aren't very useful. Here's the full data set: https://spreadsheets.google.com/ccc?hl=en&key=to6x3sukCVJ9xqTm8It73sA&hl=en#gid=2 I'm convinced now that there is value in holding off on preloading in the head to some degree in order to get to painting faster. But there are some interesting wins out there if we can preload some things in the body while blocked in the head. For instance, craigslist both loads 14% faster and paints 12% sooner and imdb loads 20% faster with no significant change in paint time. So I'm going to dig into the differences and see if we can isolate only those cases where it is always a win.
Tony Gentilcore
Comment 6 2011-01-31 14:54:26 PST
> For instance, craigslist both loads 14% faster and paints 12% sooner... Here's a cuzillion test that demonstrates craigslist's subresources (external stylesheet in head and 2 external scripts in body): http://stevesouders.com/cuzillion/?c0=hc1hfff2_0_f&c1=bj1hfff2_0_f&c2=bj1hfff2_0_f&t=1296514125 With the preload enqueueing it loads in 4 seconds, with enqueueing disabled it loads in 2. The difference is whether the script downloads are serialized or not. I would expect that at the point it gets blocked the preload scanner should be free to find the next script. I'm hunting around now to see if there's a logic bug.
Tony Gentilcore
Comment 7 2011-02-03 14:02:59 PST
> With the change, the load event is reached on average about 3% sooner, but the first paint event is moved 7% later. Jamesr and I did some digging to figure out why this pushed back painting so much while improving total load time. It turns out that what really happened is that load times were improved and paint events from the same point in parsing also improved by a similar amount. We were able to confirm this by dumping the parser's current line/col number during each paint event. The difference was that with the more aggressive preloading, earlier paint events were completely dropped. This turns out to be due to the fact that the parser yields for painting when it is blocked waiting for a script to download. With the more aggressive preloading, we eliminated points where the parser had to wait for a script to download and thus eliminated opportunities to paint. In light of this, the more aggressive preloading approach seems like a clear win, but if we want to preserve the same first paint times, we need to make the parser more yieldy. I'm playing around with some ideas for doing that and will ping back when I have something concrete.
Tony Gentilcore
Comment 8 2011-02-08 10:52:17 PST
> In light of this, the more aggressive preloading approach seems like a clear win, but if we want to preserve the same first paint times, we need to make the parser more yieldy. I'm playing around with some ideas for doing that and will ping back when I have something concrete. I was surprised that my hacked up parser which always yields before executing a script (regardless of whether it is already loaded) didn't fix the first paint delay problem. But turning on INSTRUMENT_LAYOUT_SCHEDULING in FrameView pointed out another issue. The traces from http://fashion.ebay.com/womens-clothing copied below demonstrate it. There are opportunities to paint at line 0 col 2079 and line 42 col 577. However, we don't do the first layout before 250ms. Currently we hit the first opportunity after 250ms and paint. However, with my change we hit it before 250ms, so we don't paint there and get blocked up on stylesheets again until the opportunity at line 42. Here's the current behavior: *************** Scheduling layout for 245 Stylesheet added at time 25. 1 stylesheets now remain. Stylesheet added at time 25. 2 stylesheets now remain. ENQUEUE PRELOAD http://include.ebaystatic.com:80/eboxapps/verticals/js/en_US/e695/SYS-201020_vjo_e69512468077_1_en_US.js ... 3 more enqueues here Received all data at 42 Stylesheet loaded at time 116. 1 stylesheets still remain. Stylesheet loaded at time 159. 0 stylesheets still remain. Beginning update of style selector at time 159. Finished update of style selector at time 159 Scheduling layout for 90 executeParsingBlockingScript at 160 PRELOADING http://include.ebaystatic.com/eboxapps/verticals/js/en_US/e695/verticals_Top_e69512468077_2_en_US.js ... Tons of preloads here Layout timer fired at 250 Elapsed time before first layout: 250 PAINT from [0:2079] at 254 executeParsingBlockingScript at 255 executeParsingBlockingScript at 277 Scheduling layout for 0 DidFlushPaint: set_first_paint_time on 0x462b360 ... *************** And with body scanning plus yielding: *************** Document got a renderer at 0 Received all data at 0 Scheduling layout for 247 Parsing finished at 3 Received all data at 3 Elapsed time before first layout: 4 Scheduling layout for 245 PAINT at 14 DidFlushPaint: Document got a renderer at 0 Stylesheet added at time 25. 1 stylesheets now remain. Stylesheet added at time 25. 2 stylesheets now remain. PRELOADING http://include.ebaystatic.com/eboxapps/verticals/js/en_US/e695/SYS-201020_vjo_e69512468077_1_en_US.js ... 3 more preloads here Received all data at 41 Stylesheet loaded at time 116. 1 stylesheets still remain. Stylesheet loaded at time 162. 0 stylesheets still remain. Beginning update of style selector at time 162. Finished update of style selector at time 163 Scheduling layout for 87 timerScheduled at 163 timerFired at 164 executeParsingBlockingScript at 164 timerScheduled at 164 PRELOADING http://p.ebaystatic.com/aw/pics/buy/csa/logos/sprVNLFashionVault.gif ... tons of preloads here timerFired at 173 executeParsingBlockingScript at 173 timerScheduled at 195 timerFired at 199 executeParsingBlockingScript at 199 timerScheduled at 200 timerFired at 203 executeParsingBlockingScript at 203 Stylesheet added at time 275. 1 stylesheets now remain. Stylesheet loaded at time 275. 0 stylesheets still remain. Beginning update of style selector at time 275. Finished update of style selector at time 279 Layout timer unscheduled at 279 Scheduling layout for 0 Document got a renderer at 0 Stylesheet added at time 314. 1 stylesheets now remain. Stylesheet loaded at time 314. 0 stylesheets still remain. Beginning update of style selector at time 314. Finished update of style selector at time 339 timerScheduled at 391 Layout timer unscheduled at 142 Document got a renderer at 0 Stylesheet added at time 2. 1 stylesheets now remain. Layout timer fired at 429 Elapsed time before first layout: 429 Scheduling layout for 0 PAINT from [18:106] at 71 PAINT from [42:577] at 527 timerFired at 544 executeParsingBlockingScript at 544 executeParsingBlockingScript at 546 executeParsingBlockingScript at 575 Scheduling layout for 0 Parsing finished at 612 DidFlushPaint: set_first_paint_time on 0xef22b60 ... ***************
Tony Gentilcore
Comment 9 2011-03-29 09:58:13 PDT
Created attachment 87351 [details] Patch
Tony Gentilcore
Comment 10 2011-03-30 15:28:49 PDT
Antti, you're probably the best person to review this. Would you mind taking a look when you get the chance?
Antti Koivisto
Comment 11 2011-03-30 20:29:42 PDT
Comment on attachment 87351 [details] Patch Ok, sounds convincing, r=me. It might be better if you left HTMLPreloadScanner::scanningBody() and the related tracking code still in. It can be removed later when we are sure it has no value in mobile world either. You can just do UNUSED_PARAM(referencedFromBody) with a FIXME in CachedResourceLoader::preload().
Tony Gentilcore
Comment 12 2011-03-31 09:08:53 PDT
Created attachment 87741 [details] Patch for landing
Tony Gentilcore
Comment 13 2011-03-31 09:09:20 PDT
(In reply to comment #11) > (From update of attachment 87351 [details]) > Ok, sounds convincing, r=me. > > It might be better if you left HTMLPreloadScanner::scanningBody() and the related tracking code still in. It can be removed later when we are sure it has no value in mobile world either. > > You can just do UNUSED_PARAM(referencedFromBody) with a FIXME in CachedResourceLoader::preload(). Done.
WebKit Commit Bot
Comment 14 2011-03-31 13:15:15 PDT
Comment on attachment 87741 [details] Patch for landing Rejecting attachment 87741 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 1 Last 500 characters of output: autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error [Errno 60] Operation timed out> Full output: http://queues.webkit.org/results/8315243
Antti Koivisto
Comment 15 2011-03-31 14:10:07 PDT
(In reply to comment #13) > (In reply to comment #11) > > (From update of attachment 87351 [details] [details]) > > Ok, sounds convincing, r=me. > > > > It might be better if you left HTMLPreloadScanner::scanningBody() and the related tracking code still in. It can be removed later when we are sure it has no value in mobile world either. > > > > You can just do UNUSED_PARAM(referencedFromBody) with a FIXME in CachedResourceLoader::preload(). > > Done. Oh and you should re-title the bug. Nothing changes in how images are handled. ChangeLog has some garbage: + PreloadScanner doesn&apos;t find image while executing script in head
WebKit Commit Bot
Comment 16 2011-03-31 15:14:43 PDT
Comment on attachment 87741 [details] Patch for landing Clearing flags on attachment: 87741 Committed r82628: <http://trac.webkit.org/changeset/82628>
WebKit Commit Bot
Comment 17 2011-03-31 15:14:49 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 18 2011-03-31 15:22:17 PDT
http://trac.webkit.org/changeset/82628 might have broken Chromium Linux Release
Note You need to log in before you can comment on or make changes to this bug.