Closed Bug 1129571 Opened 5 years ago Closed 5 years ago

h2/spdy ip coalescing can consider dns rrset

Categories

(Core :: Networking: HTTP, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

(Whiteboard: [spdy])

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8559326 - Flags: review?(hurley)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
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.
(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.
Attachment #8560040 - Flags: review?(hurley)
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.
Attachment #8560040 - Flags: review?(hurley) → review+
Attachment #8559326 - Attachment is obsolete: true
Attachment #8559326 - Flags: review?(hurley)
https://hg.mozilla.org/mozilla-central/rev/5d7317e09ea1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.