allow http processpending q to add more than 1

RESOLVED FIXED in mozilla14

Status

()

Core
Networking: HTTP
--
enhancement
RESOLVED FIXED
5 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)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
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
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83a04b972e9
https://hg.mozilla.org/mozilla-central/rev/e83a04b972e9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.