737155 - allow http processpending q to add more than 1

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[Patrick McManus [:mcmanus]]

processpendingq only adds one item to the network per connection entry.

with pipelining there are occasions where more than 1 might be able to go out - for instance when a connection has been validated to use pipelining for the first time. or when a connection completes on a pipeline eligible connection.

obviously more than 1 item can go onto the pipeline already out of ProcessNewTransaction() (or else it would not be pipelining) so this just gives parity when pulling from the pending queue.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

honza, if you can find a minute I'd appreciate if you could look at this one quickly so I can add it to the pipeline patch queue and get that stuff checked in. It turns out to be pretty important to one of my testing scenarios where I use a dedicated img hostname.

its also really short and easy to look at :)

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 607276 [details] [diff] [review]
patch 0

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

This is hard to review, it's ripped out of the patch queue.

Please provide bug # deps and the whole patched file or best ref to a repo where I can find this patch.

The patch looks good but I cannot review for sure w/o this.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +875,5 @@
>      nsresult rv;
> +    bool dispatchedSuccessfully = false;
> +
> +    // iterate the pending list until one is dispatched successfully. Keep
> +    // iterating afterwards only until a transaction fails to dispatch.

Could TryDispatchTransaction() potentially fail on a transaction just because of some per-class limit while some next pending transaction would potentially pass dispatch successfully?

Just if we shouldn't iterate all transactions to have a complete optimization, but the code is relatively complex and may bring performance regressions...

Let's say this is just 80% solution?

@@ +901,5 @@
> +
> +            // reset index and array length after RemoveElelmentAt()
> +            dispatchedSuccessfully = true;
> +            count = ent->mPendingQ.Length();
> +            --i;                                  /* i is signed */

|i| should be unsigned...  As well as count.

I can see this has slipped back in bug 599164 - you are changing unsigned variables back to signed [1].  That should be fixed.  I missed that during the review apparently.


[1] https://bugzilla.mozilla.org/attachment.cgi?id=597498&action=diff#a/netwerk/protocol/http/nsHttpConnectionMgr.cpp_sec5

Comment 3

[Patrick McManus [:mcmanus]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Comment on attachment 607276 [details] [diff] [review]
> patch 0
> 
> Review of attachment 607276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is hard to review, it's ripped out of the patch queue.
> 
> Please provide bug # deps and the whole patched file or best ref to a repo
> where I can find this patch.

sorry bout that. It's based on type and state 599164.

> Could TryDispatchTransaction() potentially fail on a transaction just
> because of some per-class limit while some next pending transaction would
> potentially pass dispatch successfully?
> 
> Just if we shouldn't iterate all transactions to have a complete
> optimization, but the code is relatively complex and may bring performance
> regressions...
> 
> Let's say this is just 80% solution?
> 

you've got the essence of that.. It doesn't capture everything, but only dispatching 1 is necessary for correct operation, and I didn't want to iterate the whole list in the face of failures every time.. that would actually be a pretty common case when pipelines were disabled.

Comment 4

[Patrick McManus [:mcmanus]]

ok - I've updated the signed/uint bits for i and count.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 607276 [details] [diff] [review]
patch 0

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

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

Looks good.

r=honzab

Comment 6

[Patrick McManus [:mcmanus]]

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

Comment 7

[Matt Brubeck (:mbrubeck)]

Sorry, I backed this out on inbound because something in the push appeared to cause a leak in mochitest-browser-chrome:
https://hg.mozilla.org/integration/mozilla-inbound/rev/577da08878cb

Comment 8

[Patrick McManus [:mcmanus]]

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

Comment 9

[Marco Bonardo [:mak]]

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