| Summary: | PreloadScanner doesn't find image while executing script in head | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Tony Gentilcore <tonyg> | ||||||
| Component: | WebCore Misc. | Assignee: | Tony Gentilcore <tonyg> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, ap, commit-queue, eric, jamesr, koivisto, mathias, mbelshe, mihaip, psolanki, simonjam, sreeram, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | OS X 10.5 | ||||||||
| Bug Depends on: | 54355 | ||||||||
| Bug Blocks: | 56842 | ||||||||
| Attachments: |
|
||||||||
|
Description
Tony Gentilcore
2010-09-01 17:16:19 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. I'm going to look into this next. 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. 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. 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. > 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. > 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.
> 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 ... *************** Created attachment 87351 [details]
Patch
Antti, you're probably the best person to review this. Would you mind taking a look when you get the chance? 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().
Created attachment 87741 [details]
Patch for landing
(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. 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 (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't find image while executing script in head Comment on attachment 87741 [details] Patch for landing Clearing flags on attachment: 87741 Committed r82628: <http://trac.webkit.org/changeset/82628> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/82628 might have broken Chromium Linux Release |
