Closed Bug 659760 (pipelining-review) Opened 14 years ago Closed 13 years ago

Review patches for HTTP pipelining

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mayhemer, Unassigned)

References

(Depends on 1 open bug)

Details

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
Depends on: 665885
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: 13 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.