Last Comment Bug 737155 - allow http processpending q to add more than 1
: allow http processpending q to add more than 1
Status: RESOLVED FIXED
[http-conn]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 13 Branch
: x86_64 Linux
: -- enhancement (vote)
: mozilla14
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-19 13:10 PDT by Patrick McManus [:mcmanus]
Modified: 2012-06-21 06:42 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (2.11 KB, patch)
2012-03-19 13:20 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Review

Description Patrick McManus [:mcmanus] 2012-03-19 13:10:44 PDT
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] 2012-03-19 13:20:19 PDT
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 :)
Comment 2 Honza Bambas (:mayhemer) 2012-03-19 14:48:16 PDT
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] 2012-03-19 14:59:53 PDT
(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] 2012-03-20 06:20:46 PDT
ok - I've updated the signed/uint bits for i and count.
Comment 5 Honza Bambas (:mayhemer) 2012-03-20 09:51:03 PDT
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] 2012-03-20 10:36:54 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e1d343d3a6
Comment 7 Matt Brubeck (:mbrubeck) 2012-03-20 12:59:17 PDT
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] 2012-03-22 18:01:05 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e83a04b972e9
Comment 9 Marco Bonardo [::mak] 2012-03-23 05:56:54 PDT
https://hg.mozilla.org/mozilla-central/rev/e83a04b972e9

Note You need to log in before you can comment on or make changes to this bug.