Closed Bug 659760 (pipelining-review) Opened 11 years ago Closed 10 years ago
Review patches for HTTP pipelining
This is an umbrella and helper bug to proceed with reviews for pending HTTP pipelining patches from Patrick McManus. I have following first-though plan to regroup and reorder the patches a bit to make reviewing and landing simpler: 1. First preconditions, assert failures/race condition figured out during the development: * bug 632496 - this one is a bit harder to figure correctly out, we can deal with it later * bug 631801 - we have a bit different plan to fix this -> WONTFIX?, might dep on bug 658138 * bug 615342 2. Response checking precondition: * bug 232030 * bug 597706 * bug 597684 all close to get finally reviewed and landed 3. The main pipelining enhancement core, the most important, "main part": * bug 447866 - enhancements and a lot of changes later removed/modified * bug 599164 - this one modifies a lot from the previous patch * bug 603512 - additional modifications to APIs and code Merge all these to a single patch, review it and probably also land. If necessary, discuss how to split the large patch to be landed incrementally. 4. Separate additions to the core: * bug 603514 - this one seems to me as well separated, so it probably doesn't need to be merged to the main part * bug 603508 - this needs modifications or even a new bug if necessary ; this is important part of the optimization, but let's keep it separate from the main part and the last part 5. Things to deal completely separately and probably later: * bug 602518 * bug 603505 * bug 603506 Patrick, what do you think mainly about the 3th part of this process? Do you think the patches to merge are well chosen?
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
I was talking about this privately with Patrick and the plan is following, review and land, best in this order: 1. The main pipelining enhancement core, the most important, "main part": * bug 447866 - enhancements and a lot of changes later removed/modified (first review round passed) * bug 665094 - simple patch providing an API bug 599164 is dependent on * bug 599164 - this one modifies a lot from the bug 447866 patch * bug 603512 - additional modifications to APIs and code Patrick expressed his wish to leave this separated, so let's do that that way. I have already reviewed the first patch in the line (there are issues that must be addressed) 2. Separate additions to the core: * bug 603514 * bug 632496 - happens only in conjunction with bug 603508 * bug 603508 - this needs modifications or even a new bug if necessary ; this is important part of the optimization, but let's keep it separate from the main part and the last part 3. Response checking plugins, helping to detect broken pipelined responses: * bug 232030 * bug 597684 * new bug: broken image should indicate a broken response and disable pipelining These should be landed after bug 599164, but we should be able to turn pipelining on w/o them 4. Things to deal completely separately and probably later: * bug 631801 - Patrick will update the patch, this is more an optimization * bug 602518 * bug 603505 * bug 603506 * bug 615342 - might be fixed by removing the ssl thread
This is the up to this date ordered list of patches to land, we agreed on it with Patrick: Stage 1: gain performance (things to test with Nick's NeckoNet for performance): * bug 667387 nshttppipeline object broken wrt taketransport (proposed a different solution, needs to be discussed and tested) * bug 447866 http pipelining is bursty (r+'ed !) * bug 540108 Provide basic pipeline ban API (needs re-evaluation, but is just a very simple patch needed only for simpler merging) * bug 599164 pipeline with type and state (second r round in progress, close to get finished) * bug 603512 Adjust Connection Pipeline Status for Unexpected Large Transaction Content-Length * bug 603514 restart pipelined transaction on read stall more aggressively * bug 671591 restart partial/in progress http transaction (needs a good test, pre-r'ed) * deps on making httpd.js keep-alive supportive (bug 469228) * bug 665885 Keep-Alive announced closes Stage 2: optimizations (various ways to clean and speed up things we have done in the first stage, still not for turning the pref on though): * bug 631801 stub implementation of Available() for SSL to avoid ASSSERTs (different solution proposed, patch awaited, also would be good to ask Brian to re-enable the Available API for ssl) * Bug 632496 nsHttpHandler::InPrivateBrowsingMode() breaks on non main thread * bug 603508 make nsConnectionEntry persistent for pipelines (should be based on LevelDB, IMO) * bug 602518 xhr pipeline hint (should be discussed with wider audience, priority not settled on this particular one) * Bug 672958 Have an API to explicitly set pipelining class on an HTTP channel, cleanup nsHttpTransaction::Classify() (no patch) Stage 3: get certainty (patches to allow us turn pipelining on by default, i.e. ensure as much as possible we don't display broken web pages because of pipelining): * bug 597684 Implement HTTP Assoc-req (r almost done) * no bug # Image Decoding Failures As Pipeline Feedback * bug 603505 pipeline pre-test * bug 603506 pipeline host blacklist The last two here should be more widely discussed, we need back-end for them.
(In reply to Honza Bambas (:mayhemer) from comment #2) > * bug 540108 Provide basic pipeline ban API bug 665094 is the one here...
If we are talking about pipelining can someone in "free" time look into this bug #586988 , because can be related to pipelining/request sending etc, Thanks!
(In reply to Virtual_ManPL from comment #4) > If we are talking about pipelining can someone in "free" time look into this > bug #586988 , because can be related to pipelining/request sending etc, > Thanks! I just want to be clear for the sake of the comment trail that you are suggesting pipelining might help performance on 586988. (you are not suggesting that pipelining is causing a problem in 586988).
Please keep this bug comments scoped to anything only needed for review of the patch stack, thanks.
I think the main goal of this bug has been fulfilled. There are few patches to update and some to have a first review, but we have an ordered list of the core feature patches very close to land (now actually shorter version of comment 2): Bug 667387 (landed) Bug 447866 (r-, r+) Bug 665094 (r- and disputable whether we really need it) Bug 599164 (r+) Bug 603512 (r+) Bug 603514 (r+) Bug 671591 (r+) Bug 665885 (r+) Bug 597684 (needs update) Bug 717759 (needs first review) Bug 717350 (needs first review) Remaining bugs postponed or WONTFIX'ed.
Assignee: honzab.moz → nobody
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.