88166 - subtle change in nsHttpChannel.cpp can have big impact on cached load times (?)

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[John Morrison]

So, with the jun 21st builds, there was a significant drop in page
load times on the Mac (and to a lesser degree on win32/linux), as
measured in both the page-loader tests and the ibench tests (if on a
faster machine). I had worked back to find that this was due to an
innocuous looking checkin by darin to nsHTTPChannel::ProcessNormal().

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/netwerk/protocol/http/src&command=DIFF_FRAMESET&file=nsHttpChannel.cpp&rev1=1.27&rev2=1.28&root=/cvsroot

In opt. builds on win2k and mac os9.0 I was able to back out that
simple reordering and see the cached load times increase for either
test. But the wacky thing is ... this method is _never_ called in the
case of a cached load (although that doesn't rule out some side effect
from the initial load affecting the subsequent loading times).

On jun27, another change to this method was checked into the branch
(by blizzard for darin), and this effectively unwound the effect of
the earlier checkin.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/netwerk/protocol/http/src&command=DIFF_FRAMESET&file=nsHttpChannel.cpp&rev1=1.31.2.1&rev2=1.31.2.2&root=/cvsroot

I'll attach some graphs comparing per-url loading times for builds of
jun 20, jun 21 and jun 27 (branch), on mac/linux/win32, showing the 
changes in times. A few things to note: 

 1) this effect is disproportionately greater on 'faster' machines
 2) it's also much greater on Mac than win32/linux (file system
    issues? event issues?)
 3) the improvement is only for certain URLs, which tend to be those with 
    the most images on the page (e.g., time.com, spinner.com, gamespot.com)

There are a few possibilities here, for what this all means:

  1) jrgm is on crack (or speed, as the case may be).
  2) this is measuring an artifact that does not map to real end-user 
     experience.
  3) yeah, it's faster, but it's sooo wrong it's not even funny.
  4) something is going on here that needs investigation.

Comment 1

[John Morrison]

Attachment: mac avg. cached loading times, jun 20, jun 21, jun27

Comment 2

[John Morrison]

Attachment: linux avg. cached loading times, jun 20, jun 21, jun27

Comment 3

[John Morrison]

Attachment: win98 avg. cached loading times, jun 20, jun 21, jun27

Comment 4

[John Morrison]

Doh. Should have added pavlov.

Comment 5

[Bradley Baetz (:bbaetz)]

Random, possibly incorrect guess: sending the onstart before writing into the
cache (which is what the jun 21 patch did) means content/layout/the parser can
deal with stuff earlier (since file transport stuff happens on a separate thread)

Why did some pages change, and others stayed the same, though?

John, if you revert those patches, what happens to the numbers with your
TestPageLoad thing?

Comment 6

[John Morrison]

> Random, possibly incorrect guess: sending the onstart before writing into 
> the cache (which is what the jun 21 patch did) means content/layout/the 
> parser can deal with stuff earlier (since file transport stuff happens on 
> a separate thread)

Note: 'uncached' page load times did not change. It is the 'cached' load 
times that decreased, but, to repeat, ProcessNormal() is not called in the
case of a cached load. So, you are looking for some side-effect that arises
from the reordering (sez me).

> John, if you revert those patches, what happens to the numbers with your
> TestPageLoad thing?

"John" == jtaylor

Comment 7

[John Morrison]

I just noticed that I forgot some of the series labels on those graphs. So, 
note: the red line is jun 21 on all the graphs, the blue line is jun20 and the
green line is jun27.

Comment 8

[John Taylor]

The TestPageLoad test showed no conclusive differences in timing between June
20th and today, on Windows2000.

Comment 9

[John Morrison]

jtaylor -- the TestPageLoad test ... does it exercise the cache, or is it just 
pulling content off the wire. (If the latter, then you should not see any 
change. It is only cached loads that are (apparently) affected).

Comment 10

[Patrick McManus [:mcmanus]]

this code has all be rewritten