1129571 - h2/spdy ip coalescing can consider dns rrset

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

h2/spdy coalescing as implemented requires the two potential connections to resolve to the same IP address.

We change that from equality of a single used address to be an intersection of the two DNS RR sets in question. Equality at that point is really a matter of random selection from the rr set, insisting on it doesn't give us any additional restrictive properties but it does reduce the pooling rate.

This explains some missed coalescing opportunities with both yahoo and google properties and matches chrome behavior.

Comment 1

[Patrick McManus [:mcmanus]]

 https://tbpl.mozilla.org/?tree=Try&rev=eba3207232c6

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: h2/spdy coalsescing by full DNS rrset

Comment 3

[u408661]

Comment on attachment 8559326 [details] [diff] [review]
h2/spdy coalsescing by full DNS rrset

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

::: netwerk/dns/nsIDNSRecord.idl
@@ +90,5 @@
> +
> +%{C++
> +template<class T> class nsTArray;
> +%}
> +[ref] native nsCStringTArrayRef(nsTArray<mozilla::net::NetAddr>);

This should be called nsNetAddrTArrayRef (or something similar), since it's not at all related to nsCStrings.

@@ +93,5 @@
> +%}
> +[ref] native nsCStringTArrayRef(nsTArray<mozilla::net::NetAddr>);
> +
> +[uuid(7ff197cb-1f82-4a02-ba5c-cfee98d50139)]
> +interface nsIDNSRecord2 : nsISupports

Am I missing a real reason to add a new interface here? From a quick mxr search, I don't see anything in firefox or addons that implements nsIDNSRecord other than the concrete classes you've already changed. You may know of something outside those groups that does, though, in which case, I can see the reason. Otherwise, this just seems like an unnecessary interface.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #3)
>
> > +[ref] native nsCStringTArrayRef(nsTArray<mozilla::net::NetAddr>);
> 
> This should be called nsNetAddrTArrayRef (or something similar), since it's
> not at all related to nsCStrings.

ack!

(you can tell it evolved a bit in implementation - and apparently I transcended mere symbol names in my thinking :) )

> @@ +93,5 @@
> > +%}
> > +[ref] native nsCStringTArrayRef(nsTArray<mozilla::net::NetAddr>);
> > +
> > +[uuid(7ff197cb-1f82-4a02-ba5c-cfee98d50139)]
> > +interface nsIDNSRecord2 : nsISupports
> 
> Am I missing a real reason to add a new interface here?

I have definitely seen 3rd party implementations of the resolver that I don't want to needlessly break, but I think you're right that nsIDNSRecord isn't something that gets overloaded. I'll try to consolidate.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: h2/spdy coalsescing by full DNS rrset

Comment 6

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95dc850cd981

Comment 7

[u408661]

Comment on attachment 8560040 [details] [diff] [review]
h2/spdy coalsescing by full DNS rrset

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

One thing that needs fixing before landing, but otherwise LGTM.

::: netwerk/dns/nsIDNSRecord.idl
@@ +53,5 @@
> +     *
> +     * @param aAddressArray
> +     *        The result set
> +     */
> +    [noscript] void getAddresses(out nsNetAddrTArrayRef aAddressArray);

Need to rev the uuid because of this.

Comment 8

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d7317e09ea1

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d7317e09ea1