allow http processpending q to add more than 1

RESOLVED FIXED in mozilla14

Status

()

Core
Networking: HTTP
--
enhancement
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

13 Branch
mozilla14
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [http-conn])

Attachments

(1 attachment)

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.
Created attachment 607276 [details] [diff] [review]
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 :)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #607276 - Flags: review?(honzab.moz)
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
(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.
ok - I've updated the signed/uint bits for i and count.
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
Attachment #607276 - Flags: review?(honzab.moz) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e1d343d3a6
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
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83a04b972e9
https://hg.mozilla.org/mozilla-central/rev/e83a04b972e9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Whiteboard: [http-conn]
You need to log in before you can comment on or make changes to this bug.