729133 - preconnect http sessions before cache lookup

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

As discussed at the work week, this patch implements a necko-specific speculative-connect API for use when we think a connection to a particular host will be needed "soon".

It also adds code to do this preconnect very early in the HTTP path - before the cache is consulted. That way any latency added by the cache can overlap with the connection establishment.

The connection is driven through any TCP or SSL handshakes. A "null transaction" is introduced to do the SSL portion if necessary. It is not necessary if we're not using SSL (duh) or if by the time the TCP portion of the transaction is setup there is a real transaction in the pending queue.

Currently the only case of connections that aren't paired with transactions are leftover cases from syn-retry timers firing. Currently those only do TCP handshake, but this patch extends them to do the SSL handshake as well if apropos.

An implementation side effect of this is that we do NPN for all SSL now, though if spdy is disabled we don't put spdy in the list of acceptable choices. This is because NPN is part of the path used to drive SSL when there is no transaction data to send. While its not strictly required, in practice this isn't much of a change as spdy by default is the direction we are heading and it can be argued that npn all-the-time is desirable anyhow.

my measured savings aren't impressive - but I can never reproduce any of the cache problems other people have seen. so I expect this would extend significant gains to them. Typically I see latency improvements of 2 or 3 ms, with the most extreme being 8.

Taras, you might want to measure as part of snappy?

The next step will be an hint-based idl for use by non necko things. I'll block 723628 on that work. The ability for a click in the search box to initiate a handshake has big potential imo. (especially when we get rid of the painful http->https redirection from bug 633773)

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: implementation patch 0

honza, I want to give you a break from my patch queue but I want to give you the chance to nominate someone else or claim this as a patch-of-interest for yourself. as long as we make progress, I'm good.

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 599202 [details] [diff] [review]
implementation patch 0

dropping r? as try has reported some problems... feedback welcome anyhow, of course.

Comment 3

[(dormant account)]

Patrick, any netbook should be able to do this. To reproduce a slow io situation you can either figure out how to trigger windows indexing service to run or try 
find /c/ > /dev/null
to simulate windows indexing service.

Comment 4

[(dormant account)]

I can ask QA to look at this and/or we can do a telemetry field trial.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 599202 [details] [diff] [review]
implementation patch 0

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

Do you think you could find a solution w/o the NullHT object?  I think it is there just to make the read-segments machine of nsHttpConnection spin and finish SSL nego, right?

This is what I want to fix in bug 715905.  It means to process the connection setup (proxy connect, ssl + npn nego) to a different class, independent of the transaction but joint with the callbacks (i.e. the load context) needed now, AFAIK, just for ssl client cert auth in this phase.

I think nsHalfOpenSocket could do that, could spin with PR_Write(0)'s until something different from WOULD_BLOCK is returned.  CONNECT then have to be processed there as well prior to SSL nego.

I believe that would be a good architectural change, there were some demands to change it this way.

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +419,5 @@
> +                !(caps & NS_HTTP_DISALLOW_SPDY)) {
> +                LOG(("nsHttpConnection::SetupNPN Allow SPDY NPN selection"));
> +                protocolArray.AppendElement(NS_LITERAL_CSTRING("spdy/2"));
> +            }
> +

I don't understand why these changes are needed.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1063,5 @@
>      return PL_DHASH_NEXT;
>  }
>  
> +bool
> +nsHttpConnectionMgr::RestrictConnections(nsConnectionEntry *ent)

Maybe "RestrictConcurentSPDYFreshConnections" ?

@@ +1157,1 @@
>                  return;

Please document purpose of the speculative bit on nsHalfOpenSocket.  As I understand, you want to prevent creation of a new transport (half-open) when there is a speculative load.  You drop the flag here since there is a new transaction and we are again in count(transactions) == count(connections) state.

@@ +1235,5 @@
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      ent->mHalfOpens.AppendElement(sock);
> +    if (speculative)
> +        sock->SetSpeculative(true);

Maybe just set?

Comment 6

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #5)
> Comment on attachment 599202 [details] [diff] [review]
> implementation patch 0
> 
> Review of attachment 599202 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you think you could find a solution w/o the NullHT object?  I think it is
> there just to make the read-segments machine of nsHttpConnection spin and
> finish SSL nego, right?
> 

right.. I don't think it makes fixing 715905 any harder (you basicaly need something equivalent to it to hold the same information in roughly the same places), so 715905 isn't really an argument against the null transaction. At worst you can say its a duplicate effort but what I like about it is frankly that it avoids the risk involved in moving that logic out of nsHttpConnection (my gut says that isn't as easy as it sounds - though I admit it looks doable), and we've got a whole list of other things lined up that touch this area of code that bring some end-user value instead of just architectural cleanup, so keeping those changes at bay for a while (even if all it accomplishes is easing backouts, etc) is attractive to me.

I'm not opposed to doing it overall though.

> 
> ::: netwerk/protocol/http/nsHttpConnection.cpp
> @@ +419,5 @@
> > +                !(caps & NS_HTTP_DISALLOW_SPDY)) {
> > +                LOG(("nsHttpConnection::SetupNPN Allow SPDY NPN selection"));
> > +                protocolArray.AppendElement(NS_LITERAL_CSTRING("spdy/2"));
> > +            }
> > +
> 
> I don't understand why these changes are needed.
> 

So those changes drive the SSL handshake with NPN regardless of whether spdy is configured. if spdy is not configured then spdy is not part of the selection criteria.

pragmatically, this makes the handshake pattern largely similar whether or not you're using spdy which I think is a good thing now that spdy defaults to being on. (i.e. the non-default pref changes less behavior). Brian has suggested that we should be doing npn all the time (although in this case where the default no-match is mapped to http/1 that is of limited usefulness), and most importantly it is done in this patch because it involves very minor code changes to be able to drive the handshake to completion using existing hooks if we're willing to do NPN all the time. (which we are, because spdy is default on.)

> @@ +1157,1 @@
> >                  return;
> 
> Please document purpose of the speculative bit on nsHalfOpenSocket.  As I
> understand, you want to prevent creation of a new transport (half-open) when
> there is a speculative load.  You drop the flag here since there is a new
> transaction and we are again in count(transactions) == count(connections)
> state.
> 

that's pretty much right. we might not be at balance yet, but otherwise yes.

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: implementation patch v1

fixes up the issues from try server.. released callbacks from wrong thread and some issues around CONNECT and request headers. The implementation here will actually fail on a tunnel that requires proxy-authorization.. I don't think that's a big deal as the non-speculative version run from processpendingQ will work fine.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 599460 [details] [diff] [review]
implementation patch v1

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

r- for missing test for proxy CONNECT.

::: netwerk/protocol/http/NullHttpTransaction.cpp
@@ +153,5 @@
> +    nsCAutoString hostLine;
> +    nsCString host(mConnectionInfo->GetHost());
> +    nsresult rv = nsHttpHandler::GenerateHostPort(host,
> +                                                  mConnectionInfo->Port(),
> +                                                  hostLine);

s/hostLine/hostHeader/ ?

@@ +161,5 @@
> +
> +  // CONNECT tunnels may also want Proxy-Authorization but that is a lot
> +  // harder to determine, so for now we will let those connections fail in
> +  // the NullHttpTransaction and let them be retried from the pending q
> +  // with a bound transcation

So we will waste a connection.  And if this gets grabbed for a transaction it will slow things down with restarting.  Did you test this code?  Can we have an xpcshell test (or mochitest for having an SSL) for this?  I'm worried this could break things up.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> (In reply to Honza Bambas (:mayhemer) from comment #5)
> > Comment on attachment 599202 [details] [diff] [review]
> > implementation patch 0
> > 
> > Review of attachment 599202 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Do you think you could find a solution w/o the NullHT object?  I think it is
> > there just to make the read-segments machine of nsHttpConnection spin and
> > finish SSL nego, right?
> > 
> 
> right.. I don't think it makes fixing 715905 any harder (you basicaly need
> something equivalent to it to hold the same information in roughly the same
> places), so 715905 isn't really an argument against the null transaction. 

My attitude is opposite: bug 715905 should block this one, not that bug 715905 should be blocked.  I.e. - fix bug 715905 first and then fix this one.

> At
> worst you can say its a duplicate effort but what I like about it is frankly
> that it avoids the risk involved in moving that logic out of
> nsHttpConnection (my gut says that isn't as easy as it sounds - though I
> admit it looks doable)

Yes, that is why I didn't start working on it so far.  I'm also worried it may be more work then I expect to.  But I'd like to put the effort to it anyway, sooner or later.  You see your self with your own comment in NullHttpTransaction::RequestHead() that things are not 100% OK with your current patch.

> , and we've got a whole list of other things lined up
> that touch this area of code that bring some end-user value instead of just
> architectural cleanup, so keeping those changes at bay for a while (even if
> all it accomplishes is easing backouts, etc) is attractive to me.

I understand your rapid development approach and really don't want to block it even I sometimes grate one's teeth ;)  

If there is a test for your patch to check we didn't break CONNECT and we are sure about not making a significant performance regression with first non-authenticated connections then let's put it in as it is now.

Comment 10

[Patrick McManus [:mcmanus]]

> 
> So we will waste a connection. 

sometimes. I suspect actually rather rarely because if you look at the part in nsHalfOpen where the TCP part of the handshake is complete it decides whether or not to drive SSL handshake with a null transaction based on whether or not anything is in the pending queue. If there is a transaction there then the connection goes into the persistent queue without the SSL handshake being done, is immediately picked up and paired with the pending real transaction and that sends the right proxy-authorization header.

the only reason that wouldn't happen would be if the request takes longer (due to cache IO delays?) to get to the queue than the TCP handshake. Given that the handshake here is probably to a local proxy server (and therefore quick) I guess that's a possibility, but then again if the penalty is a new connection we also know that in that case the penalty is inexpensive so no big deal.

>  Did you test this code?  Can we
> have an xpcshell test (or mochitest for having an SSL) for this?  I'm
> worried this could break things up.

I've spent a lot of time testing it by hand against squid by tweaking the above mentioned code so that it always drives the SSL handshake with the NullTransaction regardless of whether there is something pending. It works fine, and is really simpler than it sounds. There are no retries or anything like that going on to confuse channel state. The nsHttpConnection using the nullTransaction simply fails to setup the tunnel and the connection is aborted and doesn't go into the reuse pool - anytime a connection finishes the pending queue is consulted and that results in a new (paired) connection being made with the real transaction. The real transaction never sees the failure.

Unfortunately this lack of determinism makes it pretty much impossible to write a reliable mochitest. Maybe separately you could add a proxy-authorziation test to ssltunnel and we run that code enough that we'd probably see random orange if it broke? You understandably weren't thrilled with my last hackage on ssltunnel, but that's the best idea I can come up with.

I did find one other minor issue - a call to speculativeConnect() against a non-existant connectionEntry just gives up rather than making the entry - so I'll update the patch for that. And I'll make requestHead in the NUllTransaction a pointer so it can be fully-lazy.

Comment 11

[Patrick McManus [:mcmanus]]

Attachment: patch v2

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> Unfortunately this lack of determinism makes it pretty much impossible to
> write a reliable mochitest. Maybe separately you could add a
> proxy-authorziation test to ssltunnel and we run that code enough that we'd
> probably see random orange if it broke? You understandably weren't thrilled
> with my last hackage on ssltunnel, but that's the best idea I can come up
> with.

We shouldn't modify ssltunnel even more.  There start to be a queue of tests waiting for ssl enabled xpcshell tests.  I apparently have to finish it soon.  IMO, with some effort you can write a very deterministic basic xpcshell tests for this patch.

Based also on our discussion out of this bug, this may be an important advantage.  Still I'm worried this may break things while not having a test, well, that is why we have 4 channels to let the patch go through.  Also, you have spent a lot of time testing this manually.

Idea: what about to fill proxy-authorization header when request head is built on the null transaction and connection is not anonymous?  If this seems doable to you, we should do that in a followup bug.  It may need a closer look since if we screw anything we may accidentally leak credentials that way.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 600185 [details] [diff] [review]
patch v2

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

r=honzab with fingers crossed.

::: netwerk/protocol/http/NullHttpTransaction.cpp
@@ +162,5 @@
> +
> +    // CONNECT tunnels may also want Proxy-Authorization but that is a lot
> +    // harder to determine, so for now we will let those connections fail in
> +    // the NullHttpTransaction and let them be retried from the pending queue
> +    // with a bound transcation

Hm... yeah, if that is NTML then a state transition is needed to track.  Some of that nsHttpChannelAuthProvider is doing.  This may really be better implemented in bug 715905..  Needs more investigation, though.

::: netwerk/protocol/http/NullHttpTransaction.h
@@ +76,5 @@
> +  nsRefPtr<nsAHttpConnection> mConnection;
> +  nsCOMPtr<nsIInterfaceRequestor> mCallbacks;
> +  nsCOMPtr<nsIEventTarget> mEventTarget;
> +  nsRefPtr<nsHttpConnectionInfo> mConnectionInfo;
> +  nsHttpRequestHead *mRequestHead;

nsAutoPtr ?

Comment 14

[Honza Bambas (:mayhemer)]

Just a note: this has to this date a little more then two weeks to be tested on Nightly.

Josh: should we consider wait until merge to have a larger window or just go forward?

Patrick: do you have some actual numbers/telemetry that measure the benefit this patch gives us?

Comment 15

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #13)

> 
> nsAutoPtr ?

I'm on a nsAutoPtr strike because I find its assignment semantics more bug-prone than the value it gives in not leaking objects :)

(In reply to Honza Bambas (:mayhemer) from comment #14)
> Just a note: this has to this date a little more then two weeks to be tested
> on Nightly.
> 
> Josh: should we consider wait until merge to have a larger window or just go
> forward?
> 
> Patrick: do you have some actual numbers/telemetry that measure the benefit
> this patch gives us?

I'm going to go forward, plenty of time on aurora to kill all of it or maybe just the auth scenarios as they would seem to have the most risk.

As for benefit, as I say in the description it is pretty marginal for my setup (single digit milliseconds), but still measurable and positive. But I think its important for two reasons:
 1] It benefits people with bad cache performance a lot more than that - something I don't personally see, but others do have scary data on..
 2] it enables preconnect-when-you-FOO features, the first of which I hope to be "foo == click on the search box" which is pretty obviously a full rtt or even 3. so very worthwhile there :)

honza, I'm basically traveling with limited availability starting sunday and back at my desk on wednesday. I'll check this in (it has been through try) but if there's any sign of problem I'd appreciate it if you could drive the backout.

Comment 16

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f65247c7647a

Comment 17

[Geoff Lankow (:darktrojan)]

Backed out for OS X mochitest-1 orange, possibly also other m-1 and m-oth orange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/11fe119b6075

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 600185 [details] [diff] [review]
patch v2

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

Late r-, I've found out in the profiler that this patch is not doing what it has to do, check on the comments.

Note for me (again..): test before r+!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +238,5 @@
> +        NS_NewNotificationCallbacksAggregation(mCallbacks, mLoadGroup,
> +                                               getter_AddRefs(callbacks));
> +        if (callbacks)
> +            gHttpHandler->SpeculativeConnect(mConnectionInfo,
> +                                             callbacks, NS_GetCurrentThread());

This is too late.  This doesn't preconnect before opening the cache entry but after it.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +1157,1 @@
>                  return;

Hmm.. I had an idea: what about to record all halfopens in a hash table (add on setup, remove on connection done) by the URL it had been invoked for to prevent duplicate connections?  I plan to do preconnects much earlier and this simple block may not be enough.  We won't do duplicate connections for the same URL since they share the cache entry.

Comment 19

[Honza Bambas (:mayhemer)]

BTW: the connection created with the NullTransaction doesn't get reused, we open another one...

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #19)
> BTW: the connection created with the NullTransaction doesn't get reused, we
> open another one...

I've got counter evidence.. so something isn't working consistently. (assuming you really mean connection - the null transaction object can get created twice).

I'll have to follow up later on your other comments.

Comment 21

[Honza Bambas (:mayhemer)]

Hmm.. on another run I can see the preconnection works, we connect right after DNS is done.  Could it be some intermittent issue?  However, I still see two connections being made.

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 600185 [details] [diff] [review]
patch v2

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +238,5 @@
> +        NS_NewNotificationCallbacksAggregation(mCallbacks, mLoadGroup,
> +                                               getter_AddRefs(callbacks));
> +        if (callbacks)
> +            gHttpHandler->SpeculativeConnect(mConnectionInfo,
> +                                             callbacks, NS_GetCurrentThread());

My apologies, this is not too late.  Looks like I've seen results from not fully recompiled code and also my commenting was faster then my thinking, just looking at the code shows this is of course called from AsyncOpen.

However, you should actually preconnect only when opening cache entry call succeeds (i.e. we know opening is in progress and we are about to get the callback).

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #22)
> However, you should actually preconnect only when opening cache entry call
> succeeds (i.e. we know opening is in progress and we are about to get the
> callback).

IMO, we should preconnect as soon as we know the hostname to connect to, regardless of whether there is a cache entry.

HOWEVER, what about the case where we (may) have an offline cache entry? AFAICT, we should not preconnect when checking the offline cache. Ideally, if you download a game/app without an online component (e.g. a calculator), it should generate *no* network traffic in the common case.

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #23)
> IMO, we should preconnect as soon as we know the hostname to connect to,
> regardless of whether there is a cache entry.
> 
> HOWEVER, what about the case where we (may) have an offline cache entry?
> AFAICT, we should not preconnect when checking the offline cache. Ideally,
> if you download a game/app without an online component (e.g. a calculator),
> it should generate *no* network traffic in the common case.

Right, that is better.

Comment 25

[Honza Bambas (:mayhemer)]

However, it doesn't make sense to open a pre-connection when cache entry opening fails and we are going to immediately start a transaction (of course, depends on conditions).  I wanted to form my comment this way.

Comment 26

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #25)
> However, it doesn't make sense to open a pre-connection when cache entry
> opening fails and we are going to immediately start a transaction (of
> course, depends on conditions).  I wanted to form my comment this way.

I didn't look at the patch. But, I would expect that if we preconnect and then start a transaction, that transaction would use the preconnected connection, and that if there was already another connection already available, we wouldn't have created a new preconnection. So, I don't see how preconnecting and then immediately starting a transaction would hurt.

Comment 27

[Honza Bambas (:mayhemer)]

I agree.

I think in conjunction with a per-domain or per-page subdomain cache we should start preconnecting when user presses the mouse button on a link down.  It seems we may be some 100ms ahead.  On mobile may be trickier (sometimes tap means to scroll, probably not a case to preconnect).

Comment 28

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:bsmith) from comment #23)
> (In reply to Honza Bambas (:mayhemer) from comment #22)
> > However, you should actually preconnect only when opening cache entry call
> > succeeds (i.e. we know opening is in progress and we are about to get the
> > callback).
> 
> IMO, we should preconnect as soon as we know the hostname to connect to,
> regardless of whether there is a cache entry.
> 

STS, etc can change the port. Its true that this is just speculative so its ok if there is late-change like this, but unless we identify IO that is being bypassed I really don't think you are saving anything measurable. (indeed, future callers outside the http channel (e.g. clicked on the search box) will have this downside - but they are at least saving a serious amount of time.)

> HOWEVER, what about the case where we (may) have an offline cache entry?
> AFAICT, we should not preconnect when checking the offline cache. Ideally,
> if you download a game/app without an online component (e.g. a calculator),
> it should generate *no* network traffic in the common case.

the other way to look at this is that any domain using an offline cache for any reason would lose the optimization. That doesn't seem right. The apps I've seen (e.g. boston globe) definitely still use the network - they just use offline cache for pinning purposes but still do some form of freshnes validation, etc..


(In reply to Brian Smith (:bsmith) from comment #26)
> 
> I didn't look at the patch. But, I would expect that if we preconnect and
> then start a transaction, that transaction would use the preconnected
> connection, and that if there was already another connection already
> available, we wouldn't have created a new preconnection.

yes, that's basically how it works.

my plan here is just to figure out the bug that makes docshell/test/chrome/test_bug311007.xul fail intermittently and hopefully that doesn't require significant changes. I've only spent a couple hours on that so far without meaningful progress.

Comment 29

[Patrick McManus [:mcmanus]]

I've finally had a chance to figure out why the patch bounced.

There is a negative test case for https://example/ which hits a corner case where the speculative connection does not make it into the pconn pool (because the tcp connect fails), but a real connection is not tried because RestrictConnection() is still throttling connects to that host. This is sort of a special case for a SSL host that has never gotten as far as the SSL handshake.

can be fixed by processing the queue when restrictConnection() is lifted just as we process it when activeconnectionlimits() are lifted.

Comment 30

[Patrick McManus [:mcmanus]]

Attachment: patch v3

Comment 31

[Patrick McManus [:mcmanus]]

Attachment: patch v4

minor update to handle LOAD_ANONYMOUS when we know about it.

Comment 32

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #28)
> > HOWEVER, what about the case where we (may) have an offline cache entry?
> > AFAICT, we should not preconnect when checking the offline cache. Ideally,
> > if you download a game/app without an online component (e.g. a calculator),
> > it should generate *no* network traffic in the common case.
> 
> the other way to look at this is that any domain using an offline cache for
> any reason would lose the optimization. That doesn't seem right. The apps
> I've seen (e.g. boston globe) definitely still use the network - they just
> use offline cache for pinning purposes but still do some form of freshnes
> validation, etc..

I agree with Brian here.  There may be apps that don't do any networking, at least not immediately with the main load.  If we load from offline cache entry, then there must be no network access at all invoked by that channel.  If we want to optimize net access for offline apps then let's do it a different way, somewhere else, best in a followup bug.

What about LOAD_NO_NETWORK_IO, LOAD_ONLY_FROM_CACHE (and LOAD_FROM_CACHE) flags?  Those, at least the first two, should not allow this kind of preconnect as well.

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 605901 [details] [diff] [review]
patch v4

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

Note for me: except the comment on offline cache and various load flags this looks good.

Comment 34

[Patrick McManus [:mcmanus]]

Attachment: patch 5

I'm not clear on all the nuances of the offline cache. Checking mApplication cache was sufficient for me on the one in-the-wild test case I had, but if you're thinking of a different test please elaborate. Thanks.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Patrick McManus [:mcmanus] from comment #28)
> (In reply to Brian Smith (:bsmith) from comment #23)
> > IMO, we should preconnect as soon as we know the hostname to connect to,
> > regardless of whether there is a cache entry.
> 
> STS, etc can change the port.

Sorry I wasn't clear. Of course we need to know the port too. (By the way, if doing STS and/or Alternate-Protocol processing earlier helps in making the preconnect happen sooner, materially, then we should file a separate bug to move that processing earlier in the pipelining.)

> the other way to look at this is that any domain using an offline cache for
> any reason would lose the optimization. That doesn't seem right. The apps
> I've seen (e.g. boston globe) definitely still use the network - they just
> use offline cache for pinning purposes but still do some form of freshnes
> validation, etc..

AFAICT, the AppCache spec requires the browser to always refresh the manifest (bypassing the local cache), so preconnecting to the host that serves the manifest is fine. But, once we've loaded a manifest, I think there must already be some way to determine whether a particular load is covered by that manifest w.r.t. AppCache caching semantics, and when we can determine that a load would be covered by the manifest, then we should avoid the preconnecting for that load.

Comment 36

[Honza Bambas (:mayhemer)]

Comment on attachment 606688 [details] [diff] [review]
patch 5

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +367,5 @@
> +        return;
> +
> +    // Channel loads with this flags are meant to be local only
> +    if (mLoadFlags & (LOAD_ONLY_FROM_CACHE | LOAD_FROM_CACHE |
> +                      LOAD_NO_NETWORK_IO | LOAD_CHECK_OFFLINE_CACHE))

LOAD_CHECK_OFFLINE_CACHE is not enough (sorry for late reply).  

We have a flag mLoadedFromApplicationCache but that will be invalid at this time after bug 722033 gets fixed.

The proper way here is a check for mApplicationCache object presence.

However (how else ;)), we may load from net when offline cache entry has not been found (a valid case) and matches one of FALLBACK or NETWORK (BYPASS) namespaces.

Since I'd like to see this patch landed soon, we should do preconnect for network and fallback entries in a followup.  It is doable, we can check for a matching namespace sooner then after we fail to get a cache entry at [1].


Otherwise looks good, thanks.


[1] http://hg.mozilla.org/mozilla-central/annotate/8414a5a38e56/netwerk/protocol/http/nsHttpChannel.cpp#l2186

Comment 37

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #36)

> The proper way here is a check for mApplicationCache object presence.
>

SpeculativeConnect() does do that check...

+    // don't speculate on uses of the offline application cache or if
+    // we are actually offline
+    if (mApplicationCache || gIOService->IsOffline())
+        return;
+

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #37)
> (In reply to Honza Bambas (:mayhemer) from comment #36)
> 
> > The proper way here is a check for mApplicationCache object presence.
> >
> 
> SpeculativeConnect() does do that check...
> 
> +    // don't speculate on uses of the offline application cache or if
> +    // we are actually offline
> +    if (mApplicationCache || gIOService->IsOffline())
> +        return;
> +

Again, would love to have highlighting in splinter.  Then the patch is ready to land.

Comment 39

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I am finishing up the patches to bug 722034 which have merge conflicts with this one. Since you have an r+d patch already, it would be great to get your patch landed and tested for at least a few days before I attempt to land mine.

I tried to apply your patch on top of today's mozilla-central and there were several conflicts in nsHttpConnectionMgr.cpp and nsHttpConnectionMgr.h. If you are landing this patch this week, then I will rebase my patches on top of it; either way, please let me know what you're planning.

Comment 40

[Patrick McManus [:mcmanus]]

hey brian, I know this patch is bitrotted already which is why I haven't landed it yet. And I'm going on vacation this week, so you should feel free to proceed with whatever you're doing and I'll sort out the pieces when I'm back. It will catch whatever train it is ready for.

thanks!

Comment 41

[Patrick McManus [:mcmanus]]

I've de-bitrotted this and now that mozilla-central is open again I'd like to land it as it looks like 722034 has some time to go to get the necessary reviews.. I think the only conflict is probably the couple of lines in nshttpchannel.cpp, right? That should be easy enough to sort out.

let me know if that's a problem. I'm running it through try again to just be sure and try has a huge (1000+) backlog, so you've got a little time to answer :)

Comment 42

[Patrick McManus [:mcmanus]]

Attachment: patch 6

fyi updated for bitrot

Comment 43

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c773ee0f0245

Comment 44

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/c773ee0f0245

Comment 45

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 618005 [details] [diff] [review]
patch 6

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

::: netwerk/protocol/http/NullHttpTransaction.cpp
@@ +87,5 @@
> +                                           nsIEventTarget **outTarget)
> +{
> +  nsCOMPtr<nsIInterfaceRequestor> copyCB(mCallbacks);
> +  *outCB = copyCB;
> +  copyCB.forget();

Ugh. This should be copyCB.forget(outCB);

@@ +92,5 @@
> +
> +  if (outTarget) {
> +    nsCOMPtr<nsIEventTarget> copyET(mEventTarget);
> +    *outTarget = copyET;
> +    copyET.forget();

Same here