Last Comment Bug 792438 - Make sure that we load stylesheets and scripts with a higher priority than images
: Make sure that we load stylesheets and scripts with a higher priority than im...
Status: RESOLVED FIXED
[Snappy]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 1 vote (vote)
: mozilla20
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
: 813712 (view as bug list)
Depends on: 818711 816685 818962 857906 858605 862382 864014
Blocks: 513564 813707 813712
  Show dependency treegraph
 
Reported: 2012-09-19 08:22 PDT by :Ehsan Akhgari
Modified: 2013-04-20 06:59 PDT (History)
24 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 0 imglib proxy load group fix rev 0 (1.30 KB, patch)
2012-11-29 13:36 PST, Patrick McManus [:mcmanus]
joe: review+
Details | Diff | Splinter Review
part 1 have content flag http channels for blocking and unblockables rev 0 (23.56 KB, patch)
2012-11-29 13:42 PST, Patrick McManus [:mcmanus]
bzbarsky: review+
Details | Diff | Splinter Review
part 2 network implementation of blocking/unblockable rev 0 (42.30 KB, patch)
2012-11-29 13:49 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
part 3 killswitch rev 0 (6.32 KB, patch)
2012-12-03 14:34 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review
part 2 network implementation of blocking/unblockable rev 1 (42.31 KB, patch)
2012-12-04 10:29 PST, Patrick McManus [:mcmanus]
mcmanus: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-09-19 08:22:30 PDT
I'm sort of stepping into unknown territory here, but Jeff and I were looking at tests at webpagetest.org (sample: <http://www.webpagetest.org/result/120919_DK_P4Z/1/details/>) and sometimes it seems that we try to load images sooner than Chrome does.  I don't know what our heuristics in the load priorities look like, but it seems to me that images (at least those which won't affect layout) should have a lower priority compared to stylesheets and scripts in order for us to be able to get something to the screen faster.

Boris, do you know how we prioritize these types of loads?
Comment 1 Boris Zbarsky [:bz] 2012-09-19 08:30:58 PDT
Sort of, yes.

Stylesheets and scripts just get default priority.

Images start off with PRIORITY_LOW in NewImageChannel over in imgLoader.cpp, then once the image frame is created we bump the priority for that image by a -1.  Note that NORMAL is 0 and LOW is 10, so it would take a lot of -1s to get down below NORMAL.

Now the thing is, priorities only affect stuff when necko has to pick the next request to put on the network.  So if a lower-priority load starts first, when we have an open connection slot, it'll go immediately; a higher-priority load that comes along at that point will have to wait.

Now for the particular page and waterfall diagram in question, which exact part are we worried about?  All the jpg files that happen before load.min.js?  Something else?

(Note: we do speculative preload of images, just like for scripts and stylesheets; I'm not sure WebKit does, which could affect when they start their image loads.)
Comment 2 :Ehsan Akhgari 2012-09-19 08:58:13 PDT
(In reply to comment #1)
> Images start off with PRIORITY_LOW in NewImageChannel over in imgLoader.cpp,
> then once the image frame is created we bump the priority for that image by a
> -1.  Note that NORMAL is 0 and LOW is 10, so it would take a lot of -1s to get
> down below NORMAL.

I see.

> Now the thing is, priorities only affect stuff when necko has to pick the next
> request to put on the network.  So if a lower-priority load starts first, when
> we have an open connection slot, it'll go immediately; a higher-priority load
> that comes along at that point will have to wait.

Hmm, that is something that I did not expect.  Correct me if I'm wrong, but does that mean that if we have one connection slot open and the parser sees an <img> and then a <link rel=stylesheet>, the stylesheet should wait until the image has been downloaded (or another slot has finished downloading)?

> Now for the particular page and waterfall diagram in question, which exact part
> are we worried about?  All the jpg files that happen before load.min.js? 
> Something else?

Well, compare it to <http://www.webpagetest.org/result/120919_FC_QV5/1/details/>.  It seems like Chrome is not in a rush to load images, which I could imagine would lead to it be able to put things on the screen faster.

> (Note: we do speculative preload of images, just like for scripts and
> stylesheets; I'm not sure WebKit does, which could affect when they start their
> image loads.)

What does speculative image loading mean?  Also, do we differentiate between images which affect layout and those that don't?
Comment 3 Boris Zbarsky [:bz] 2012-09-19 09:35:19 PDT
> does that mean that if we have one connection slot open and the parser sees an <img> and
> then a <link rel=stylesheet>, the stylesheet should wait until the image has been
> downloaded (or another slot has finished downloading)?

s/should/will/ and yes, as currently implemented.

> which I could imagine would lead to it be able to put things on the screen faster.

That depends on the images and their interaction with stuff around them.

You could try turning off speculative image preloading and seeing how it affects things.  Just make nsDocument::MaybePreloadImage return without doing anything and see what happens?

> What does speculative image loading mean? 

Consider this markup:

  <script src="foo"></script>
  <img src="bar">

We will start the fetch for the image before we have executed the script.  I think WebKit does in some cases (e.g. http://www.browserscope.org/network/test suggests it does), but perhaps not others?

Perhaps what we really want is to tune the speculative load stuff somehow...

> Also, do we differentiate between images which affect layout and those that don't?

Apart from testing whether the image has a frame at all, no.
Comment 4 :Ehsan Akhgari 2012-09-19 10:18:28 PDT
(In reply to comment #3)
> > does that mean that if we have one connection slot open and the parser sees an <img> and
> > then a <link rel=stylesheet>, the stylesheet should wait until the image has been
> > downloaded (or another slot has finished downloading)?
> 
> s/should/will/ and yes, as currently implemented.

OK.  Is there an easy way that we can somehow optimize this, like tell Necko to load lower priority stuff when we don't have anything to parse so won't possibly discover new scripts and styles to load?

> > which I could imagine would lead to it be able to put things on the screen faster.
> 
> That depends on the images and their interaction with stuff around them.
> 
> You could try turning off speculative image preloading and seeing how it
> affects things.  Just make nsDocument::MaybePreloadImage return without doing
> anything and see what happens?

Well we should definitely do that when someone figures out how to run that test suite locally...  I don't know of a good way to test the impact of that change without that.

> > What does speculative image loading mean? 
> 
> Consider this markup:
> 
>   <script src="foo"></script>
>   <img src="bar">
> 
> We will start the fetch for the image before we have executed the script.  I
> think WebKit does in some cases (e.g. http://www.browserscope.org/network/test
> suggests it does), but perhaps not others?

Oh, I see.  OK.

> Perhaps what we really want is to tune the speculative load stuff somehow...

Probably, although I don't think I have any good ideas on how to do that.

> > Also, do we differentiate between images which affect layout and those that don't?
> 
> Apart from testing whether the image has a frame at all, no.

Is that a useful optimization, like by assigning lower priority to background images and those which have a width and height attribute on their img element?
Comment 5 Boris Zbarsky [:bz] 2012-09-19 10:39:37 PDT
> OK.  Is there an easy way that we can somehow optimize this

Enter Patrick.  ;)  Patrick, thoughts on how we can set this up better?

> Is that a useful optimization, like by assigning lower priority to background images
> and those which have a width and height attribute on their img element?

Background images have no frame, so automatically get the PRIORITY_LOW.

Images with a width and height attribute we could avoid bumping, yes.  It _might_ be useful.

A lot of this stuff is in the realm of voodoo, not science, right now.  :(
Comment 6 Patrick McManus [:mcmanus] 2012-09-19 13:57:42 PDT
I know from conversations with google and ms engineers that both ie and chrome delay fetching images.. but I don't know the details of how long or until what condition is met. from talking to folks the reasons for that are related both to reflow, and perhaps more importantly, the image data tends to crowd out the more important css/js/html data on the wire and also eat up the 6 slots for parallel connections to the server that might be needed for more js that is discovered late. But of course a balance needs to be struck.

I actually tossed this topic into the idea hopper at the netwerk work week and it didn't make the near term goals cut. I didn't push on it too hard yet because we don't have a great framework for evaluating the approaches to this kind of prioritization problem yet - but I think that inter-module cooperation on this stuff is one of the most promising things we can be doing.

the necko team is building stoneridge - which benchmarks transport time of a set of resources across a variety of networks (broadband, mobile, etc..) it is xpcshell-on-neckonet based and therefore is going to have some shortcomings in evaluating any changes to a problem like this. Talos can do the whole page issues pretty well, but only runs on localhost and iirc actually runs everything through a proxy layer which changes some of the networking choices... none of that is unsolveable - but unless it is solved its hard to graduate past voodoo.

My hunch on the right thing to do is to limit the number of outstanding image requests to 2 or 3 whenever a higher priority request is concurently in progress. 

More speculatively, I'd actually like to be able to prioritize (and re-prioritize, a concept we don't have right now) for images that might be in the viewport vs stuff that we know is likely not in the viewport. Images deemed out of the viewport might even pick up an outright delay of a few dozen milliseconds to let everything else shake out.

Its my hope that spdy can actually reduce the number of reflows because it can have prioritized parallelization for large numbers of resources.. hopefully that means you would get the leading bytes of images much earlier in the page load because each image wouldn't be serialized (or parallelized in small groups). I haven't totally seen this proved out yet.
Comment 7 Boris Zbarsky [:bz] 2012-09-19 19:49:11 PDT
> My hunch on the right thing to do is to limit the number of outstanding image requests
> to 2 or 3 whenever a higher priority request is concurently in progress. 

Hmm.  So that's an interesting point.  We could simply limit the number of in-flight speculative image fetches we allow per page on the DOM side.  We'd still kick off a few images here, though...

"might be in the viewport" is hard to do here, by the way.  If you look at those charts linked above, the hplib-min.css, jquery-1.7.2-min.js, hplib-min.js and scriptaculous.1.8.2.min.js are all in the <head>, so they all have to be downloaded and parsed or executed before we can get any layout information whatsoever.  Now notice all the network loads that complete way before we get those four things downloaded...

Speaking of which, looking at those charts we actually get to content render before Chrome, and to "DOM complete" at about the same time.  Assuming they're charting apples and apples, of course.
Comment 8 :Ehsan Akhgari 2012-09-19 20:09:29 PDT
(In reply to comment #6)
> the necko team is building stoneridge - which benchmarks transport time of a
> set of resources across a variety of networks (broadband, mobile, etc..) it is
> xpcshell-on-neckonet based and therefore is going to have some shortcomings in
> evaluating any changes to a problem like this. Talos can do the whole page
> issues pretty well, but only runs on localhost and iirc actually runs
> everything through a proxy layer which changes some of the networking
> choices... none of that is unsolveable - but unless it is solved its hard to
> graduate past voodoo.

This sounds very good!  Is there a bug or something that we can follow along?

> My hunch on the right thing to do is to limit the number of outstanding image
> requests to 2 or 3 whenever a higher priority request is concurently in
> progress. 

That seems like a good idea at this point.  What do you think, Boris?

> More speculatively, I'd actually like to be able to prioritize (and
> re-prioritize, a concept we don't have right now) for images that might be in
> the viewport vs stuff that we know is likely not in the viewport. Images deemed
> out of the viewport might even pick up an outright delay of a few dozen
> milliseconds to let everything else shake out.

That is a great idea, but as I understand things, we don't have all of the infrastructure in place to tell whether an image is in the viewport.  Also, the fact that we need to lay out the page to know whether something is in the viewport makes things more complicated.
Comment 9 Boris Zbarsky [:bz] 2012-09-19 20:11:55 PDT
> That seems like a good idea at this point.  What do you think, Boris?

See above. Game to try, would like to understand how we'll know we have achieved whatever our goal is.
Comment 10 Patrick McManus [:mcmanus] 2012-09-19 20:23:05 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #8)
> (In reply to comment #6)
> > the necko team is building stoneridge - which benchmarks transport time of a
> > set of resources across a variety of networks (broadband, mobile, etc..) it is
> > xpcshell-on-neckonet based and therefore is going to have some shortcomings in
> > evaluating any changes to a problem like this. Talos can do the whole page
> > issues pretty well, but only runs on localhost and iirc actually runs
> > everything through a proxy layer which changes some of the networking
> > choices... none of that is unsolveable - but unless it is solved its hard to
> > graduate past voodoo.
> 
> This sounds very good!  Is there a bug or something that we can follow along?

Josh and Nick have been spearheading the stoneridge work and can provide more info.. some good stuff is here https://wiki.mozilla.org/Necko/Performance/AutomatedTesting
Comment 11 :Ehsan Akhgari 2012-09-19 20:35:17 PDT
(In reply to Boris Zbarsky (:bz) from comment #7)
> Speaking of which, looking at those charts we actually get to content render
> before Chrome, and to "DOM complete" at about the same time.  Assuming
> they're charting apples and apples, of course.

Yes, but we will display things more slowly than Chrome does.  The point here is that if our interruptible layout can only lay a small number of things out because of loading scripts for example, our rendering progression on the screen looks worse.
Comment 12 Patrick McManus [:mcmanus] 2012-11-20 13:05:57 PST
I think the initial policy I'd like to try out here will be "block non html/js/css when js/css from head is being loaded"
Comment 13 Henri Sivonen (:hsivonen) 2012-11-27 23:50:50 PST
(In reply to Patrick McManus [:mcmanus] from comment #12)
> I think the initial policy I'd like to try out here will be "block non
> html/js/css when js/css from head is being loaded"

Is there a particular reason to pay attention to “JS/CSS from head” as opposed to “JS/CSS seen before the first img”? That is, why make </head> magic instead of just caring about the order?

If there is a good reason to make </head> magic, the way to proceed would be adding eSpeculativeLoadScriptFromHead and eSpeculativeLoadStyleFromHead opcodes to nsHtml5SpeculativeLoad and then checking in nsHtml5TreeBuilder::createElement whether (mode == NS_HTML5TREE_BUILDER_IN_HEAD) and choosing the opcode variant based on that.

But if there isn’t a strong reason to make </head> magic, the first call to nsHtml5TreeOpExecutor::PreloadImage could be the signal of the end of priority JS/CSS.
Comment 14 Patrick McManus [:mcmanus] 2012-11-28 07:21:10 PST
(In reply to Henri Sivonen (:hsivonen) from comment #13)
> (In reply to Patrick McManus [:mcmanus] from comment #12)
> > I think the initial policy I'd like to try out here will be "block non
> > html/js/css when js/css from head is being loaded"
> 
> Is there a particular reason to pay attention to “JS/CSS from head” as
> opposed to “JS/CSS seen before the first img”? That is, why make </head>
> magic instead of just caring about the order?
>

Henri - thanks! I might have more questions but that gets me going.

<head><script impt></head>
<body>
<script analytics>
<img footer>
</body>

Ideally In the case above - "impt" should not load in parallel with "analytics" .. also "analytics" should load in parallel with "footer".

I think keying that off of head makes this work, but keying it off of first image does not.
Comment 15 Patrick McManus [:mcmanus] 2012-11-28 10:21:35 PST
*** Bug 813712 has been marked as a duplicate of this bug. ***
Comment 16 Patrick McManus [:mcmanus] 2012-11-28 11:38:38 PST
Some early promising returns from the rev=0 patchset (to be posted pending some try runs):

"wait time" is the time from blank screen to having an interesting paint happen that my brain can begin to parse. I measured it with a stop watch over a few iterations. "page load time" is the usual thing measured by talos. Times in seconds. This is my home broadband on win7 - local conditions matter a lot!

                wait-time-control plt-control  wait-time-patched plt-patched
pinterest.com         3.4            1.6             5.1            4.7
cnn.com               2.3            3.9             2.1            3.9
163.com               4.9            13.1            2.4           12.5

I tossed 163.com into the mix because it is a more extreme version of pinterest (huge amounts of parallel media). cnn is there as a kind of vanilla data point.

wait-time is what we're looking for here while hoping not to do too much damage to plt - looks good so far. Improvements to plt are likely due to reduced collisions/retransmissions in the html/js/css phase and sites that aren't as over parallelized won't see that.
Comment 17 Patrick McManus [:mcmanus] 2012-11-28 11:44:58 PST
I transposed a couple numbers in the pinterest results.. these are the right ones.
 
                 wait-time-control plt-control  wait-time-patched plt-patched
pinterest.com         3.4            5.1             1.6            4.7
cnn.com               2.3            3.9             2.1            3.9
163.com               4.9            13.1            2.4           12.5
Comment 18 Patrick McManus [:mcmanus] 2012-11-29 13:36:33 PST
Created attachment 686774 [details] [diff] [review]
part 0 imglib proxy load group fix rev 0

This is part 1 (of what looks like 3 parts to me now).

Necko needs to be able to group related requests together in order to do this prioritization and the way to do that is with the nsILoadGroup all the requests for the same page should share.. bug 421128 would need a similar concept.

image lib uses a loadgroup proxy which actually presents a different loadgroup object to necko than the html/js/css.

Fortunately nsILoadGroup lets you chain these things together, which is what this patch does for imagelib (i.e. it sets the original loadgroup as the root of the proxy).. necko then just uses the base loadgroup in the chain as the identifier.

As far as imagelib is concerned this shouldn't change what group gets cancel notifications etc.. it should be transparent.
Comment 19 Patrick McManus [:mcmanus] 2012-11-29 13:42:08 PST
Created attachment 686778 [details] [diff] [review]
part 1 have content flag http channels for blocking and unblockables rev 0

This patch uses 2 new bools on nsIHttpInternalChannel to flag requests that should block other requests without the flag from being added to the network until they are complete. 

css and head-js use the flag.

There is also a bool to indicate that you should be able to share the channel at all times, even if "blockers" are present. (other blockers inherently have this property). This property is given to xhr and websockets as they might be used by some of the aforementioned blocking scripts. This is mostly out of an abundance of caution of someone writing an overly clever deadlocking scenario.
Comment 20 Patrick McManus [:mcmanus] 2012-11-29 13:49:23 PST
Created attachment 686780 [details] [diff] [review]
part 2 network implementation of blocking/unblockable rev 0

last part of series.

this is the necko implementation of blocking/unblockable.. 

we allow a small quota of syns to happen in parallel with blocking-phase transfers.. the small amount of bandwidth is an acceptable price for the future latency savings, but traces showed we don't want to make that unlimited. (e.g. an unlimited flow of syns for pinterest drove the RTT up to 200ms during the css transfer, but limiting it to 6 + the blockers pushed that back down to ~35ms while still setting up many hot connections for use when the media transfer phase began.)

spdy's ineherently muxxed and prioritized qualities should work better here than this approach, so the blocking phase is not implemented for spdy.
Comment 21 Boris Zbarsky [:bz] 2012-11-30 14:22:44 PST
Comment on attachment 686778 [details] [diff] [review]
part 1 have content flag http channels for blocking and unblockables rev 0

>+++ b/content/base/src/nsScriptLoader.cpp
>+nsScriptLoader::StartLoad(nsScriptLoadRequest *aRequest, const nsAString &aType,
>+                          bool scriptFromHead)

aScriptFromHead, both here and for nsScriptLoader::PreloadURI.

>@@ -513,17 +524,17 @@ nsScriptLoader::ProcessScriptElement(nsI
>+      rv = StartLoad(request, type, false);

That third arg should be based on whether the element is a child of <head>, no?  Or do we hit the preload codepath even for the first script on a page?

>+++ b/content/base/src/nsScriptLoader.h
>+   * @param aType Whether or not the script load was keyed off of a head
>+   *                     element

aScriptFromHead, not aType.  And perhaps "whether the script was a child of <head>".  Or "descendant of <head>" if that's what the boolean really means.

>+                     bool scriptFromHead);

aScriptFromHead

>+++ b/content/base/src/nsXMLHttpRequest.cpp
>+    internalHttpChannel->SetLoadUnblocked(true);

Worth adding a comment here explaining why we're doing the unblocked bit.

>+++ b/layout/style/Loader.cpp
>@@ -1497,16 +1498,21 @@ Loader::LoadSheet(SheetLoadData* aLoadDa
>+      internalHttpChannel->SetLoadAsBlocking(true);

This should perhaps pass !aLoadData->mWasAlternate.

>+++ b/netwerk/protocol/http/nsIHttpChannelInternal.idl

Need to rev the IID.

>+    attribute boolean loadAsBlocking;
>+    attribute boolean loadUnblocked;

I'm not entirely happy with the self-explanatory power of those names, but I haven't been able to come up with better ones.  :(


>+++ b/netwerk/protocol/websocket/WebSocketChannel.cpp
>+  rv = mChannel->SetLoadUnblocked(true);

Again, document.

>+++ b/parser/html/nsHtml5SpeculativeLoad.h
>+                           bool parserInHead) {

aParserInHead.

>+++ b/parser/html/nsHtml5TreeOpExecutor.cpp
>+                                     bool scriptFromHead)

aScriptFromHead.

>+++ b/parser/html/nsHtml5TreeOpExecutor.h
>+                       bool scriptFromHead);

And here.

r=me
Comment 22 William Chan 2012-12-01 18:21:04 PST
Glad to see y'all are doing this! And if https://bugzilla.mozilla.org/show_bug.cgi?id=792438#c20 is right, you are going to beat us to removing this delay for SPDY, which is awesome. Also, for added support for this change, you can see the numbers we've released here (https://code.google.com/p/chromium/issues/detail?id=157763) which indicate the changes in PLT and first paint times we've seen in synthetic tests (WebPageTest) in aggregate across top sites.

That idea you guys floated about allowing maybe 2 or so images to load is an interesting one. Patrick, if you get data on that, let me know. We're still working on moving that logic out of WebKit into Chromium proper (https://codereview.chromium.org/11270027/), but once that is done we'll experiment with this and get data and report back.
Comment 23 Patrick McManus [:mcmanus] 2012-12-03 14:05:22 PST
(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 686778 [details] [diff] [review]
> part 1 have content flag http channels for blocking and unblockables rev 0
> 
> >@@ -513,17 +524,17 @@ nsScriptLoader::ProcessScriptElement(nsI
> >+      rv = StartLoad(request, type, false);
> 
> That third arg should be based on whether the element is a child of <head>,
> no?  Or do we hit the preload codepath even for the first script on a page?
> 

yes the first script is included in the preload list..

thanks!
Comment 24 Patrick McManus [:mcmanus] 2012-12-03 14:34:53 PST
Created attachment 687964 [details] [diff] [review]
part 3 killswitch rev 0
Comment 25 Boris Zbarsky [:bz] 2012-12-03 16:57:34 PST
> yes the first script is included in the preload list..

What about cases like this:

  <head>
    <script>document.write("<script src='foo'></" + "script>");</script>
  </head>

?  Or do we not care too much?
Comment 26 Honza Bambas (:mayhemer) 2012-12-04 09:03:49 PST
Comment on attachment 686780 [details] [diff] [review]
part 2 network implementation of blocking/unblockable rev 0

Review of attachment 686780 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab but please address the comment.

::: netwerk/base/public/nsILoadGroup.idl
@@ +95,5 @@
> +    /**
> +     * Increase the number of active blocking channels associated
> +     * with this load group by one.
> +     */
> +    void addBlockingChannel();

Since you are working with transactions here, it should be named so.

::: netwerk/base/src/nsLoadGroup.cpp
@@ +1013,5 @@
> +NS_IMETHODIMP
> +nsLoadGroupConnectionInfo::GetBlockingChannelCount(uint32_t *aBlockingChannelCount)
> +{
> +    NS_ENSURE_ARG_POINTER(aBlockingChannelCount);
> +    *aBlockingChannelCount = static_cast<uint32_t>(mBlockingChannelCount);

use PR_ATOMIC_SET to get atomically?

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +681,5 @@
>      return rv;
>  }
>  
> +void
> +nsHttpChannel::SetupTransactionLoadGroup()

SetupTransactionLoadGroupInfo()

@@ +687,5 @@
> +    // Find the loadgroup at the end of the chain in order
> +    // to make sure all channels derived from the load group
> +    // use the same connection scope.
> +    nsCOMPtr<nsILoadGroup> rootLoadGroup = mLoadGroup;
> +    nsCOMPtr<nsILoadGroup> tmp;

Move |tmp| definition into the while() body

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +736,5 @@
>          return PL_DHASH_STOP;
>  
>      return PL_DHASH_NEXT;
>  }
> +PLDHashOperator

blank line before please.

@@ +2415,3 @@
>          !AtActiveConnectionLimit(ent, trans->Caps())) {
>          CreateTransport(ent, trans, trans->Caps(), true);
>      }

An important one: please add a log why we have skipped call to CreateTransport().  I want to avoid or make simpler to fix issues with hanging transactions this could potentially cause.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1143,5 @@
> +    // for starting a new speculative connection.
> +    if (PREF_CHANGED(HTTP_PREF("speculative-parallel-limit"))) {
> +        rv = prefs->GetIntPref(HTTP_PREF("speculative-parallel-limit"), &val);
> +        if (NS_SUCCEEDED(rv))
> +            // the pref is in seconds, but the variable is in milliseconds

Fix the comment please.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1508,5 @@
> +{
> +    if (mDispatchedAsBlocking)
> +        return;
> +
> +    LOG(("nsHttpTransaction %p dispatched as blocking\n", this));

I think this log is redundant, but up to you if you anticipate missing CI often.

@@ +1525,5 @@
> +nsHttpTransaction::RemoveDispatchedAsBlocking()
> +{
> +    nsCOMPtr<nsILoadGroupConnectionInfo> saveme = mLoadGroupCI;
> +    ReleaseBlockingTransaction();
> +    mLoadGroupCI = saveme;

I'd turn it around: let RemoveDispatchedAsBlocking() implement what ReleaseBlockingTransaction() does and let ReleaseBlockingTransaction() just call RemoveDispatchedAsBlocking() and then release CI.
Comment 27 Honza Bambas (:mayhemer) 2012-12-04 09:07:20 PST
Comment on attachment 687964 [details] [diff] [review]
part 3 killswitch rev 0

Review of attachment 687964 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

::: modules/libpref/src/init/all.js
@@ +915,5 @@
>  pref("network.http.speculative-parallel-limit", 6);
>  
> +// Whether or not to block requests for non head js/css items (e.g. media)
> +// while those elements load.
> +pref("network.http.blocking-pageloads", true);

better name could be "control-requests-prioritization" or so.  blocking-pageloads is IMO misleading.
Comment 28 Honza Bambas (:mayhemer) 2012-12-04 09:16:17 PST
(In reply to Honza Bambas (:mayhemer) from comment #27)
> better name could be "control-requests-prioritization" or so. 
> blocking-pageloads is IMO misleading.

or "rendering-critical-requests-prioritization"
Comment 29 Patrick McManus [:mcmanus] 2012-12-04 09:29:06 PST
(In reply to Boris Zbarsky (:bz) from comment #25)
> > yes the first script is included in the preload list..
> 
> What about cases like this:
> 
>   <head>
>     <script>document.write("<script src='foo'></" + "script>");</script>
>   </head>
> 
> ?  Or do we not care too much?

as a first cut, I don't care too much :)

But I care a little - so I coded that up (just checking non preloaded script ancestors for head elements and screening out script elements that had async set) and found some scripts I didn't want to block on were marked blocking.

e.g. in http://www.yelp.com/ they have some javascript inlined at the bottom of the <body> section that creates new script elements and appends it an existing script element in the head.. that's normally how ga.js is done, right? Most of the time these things seem to set async, but not always.. on yelp.com that results in all of the following being marked as "flag as blocking loading" even though they are generated from scripts trailing the html (which I take to mean as the authors intent not to block anything on them).

http://ajax.googleapis.com/ajax/libs/jquery/1.5.2/jquery.min.js 
http://s3-media4.ak.yelpcdn.com/assets/2/www/js/c89b7e15c217/assets/module_core-en_US.min.js
http://s3-media4.ak.yelpcdn.com/assets/2/www/js/191490c70f84/assets/module_logged_out-en_US.min.js
http://s3-media4.ak.yelpcdn.com/assets/2/www/js/dee9bba9e0f0/assets/module_home-en_US.min.js
http://b.scorecardresearch.com/beacon.js

can you think of a simple way to address that?

I don't think its a problem to proceed with "don't block on things not from the preload list" and wait to see if we have use cases that aren't capturing the right stuff - its certainly more conservative than an algorithm that might capture too much.
Comment 30 Boris Zbarsky [:bz] 2012-12-04 09:38:49 PST
> and found some scripts I didn't want to block on were marked blocking.

Aha!  I was wondering whether doing this would be better or worse; sounds like it's worth.

Can you just add a comment saying we explicitly don't want things that wouldn't get preloaded treated this way, with a link to this bug?

Of course this means that scriptloaders that do all their work from script won't get the benefits of this bugfix, but I don't see a great way around that.

> can you think of a simple way to address that?

The only way I can think of is a boolean on the document or something that indicates that we've seen </head> already.
Comment 31 Patrick McManus [:mcmanus] 2012-12-04 10:23:01 PST
thanks honza - one question:

(In reply to Honza Bambas (:mayhemer) from comment #26)
>
> ::: netwerk/base/src/nsLoadGroup.cpp
> @@ +1013,5 @@
> > +NS_IMETHODIMP
> > +nsLoadGroupConnectionInfo::GetBlockingChannelCount(uint32_t *aBlockingChannelCount)
> > +{
> > +    NS_ENSURE_ARG_POINTER(aBlockingChannelCount);
> > +    *aBlockingChannelCount = static_cast<uint32_t>(mBlockingChannelCount);
> 
> use PR_ATOMIC_SET to get atomically?
>

I'm not sure I follow..
how is a 32 bit read not atomic? 
and how would I use _SET to get it? (there is not _GET)
Comment 32 Patrick McManus [:mcmanus] 2012-12-04 10:29:00 PST
Created attachment 688346 [details] [diff] [review]
part 2 network implementation of blocking/unblockable rev 1

Boris, can you take a quick look at the nsILoadGroup change in here?
Comment 33 Boris Zbarsky [:bz] 2012-12-04 11:40:30 PST
Comment on attachment 688346 [details] [diff] [review]
part 2 network implementation of blocking/unblockable rev 1

Looks reasonable.
Comment 34 Honza Bambas (:mayhemer) 2012-12-04 12:11:39 PST
(In reply to Patrick McManus [:mcmanus] from comment #31)
> I'm not sure I follow..
> how is a 32 bit read not atomic? 
> and how would I use _SET to get it? (there is not _GET)

You can use PR_ATOMIC_SET with passing the existing value (unlocked) as a value to set.  The function returns previous value before it has been set, so you actually read it.  But I also believe 32 bit reads are atomic.  So probably nothing to do here.
Comment 37 :Ehsan Akhgari 2012-12-05 15:21:52 PST
I filed bug 818711 with an idea on how to measure the impact of this change.

Note You need to log in before you can comment on or make changes to this bug.