Closed
Bug 659760
(pipelining-review)
Opened 14 years ago
Closed 13 years ago
Review patches for HTTP pipelining
Categories
(Core :: Networking: HTTP, defect)
Core
Networking: HTTP
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?
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Reporter | ||
Comment 1•13 years ago
|
||
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
Reporter | ||
Comment 2•13 years ago
|
||
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.
Reporter | ||
Comment 3•13 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #2)
> * bug 540108 Provide basic pipeline ban API
bug 665094 is the one here...
Comment 4•13 years ago
|
||
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!
Comment 5•13 years ago
|
||
(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).
Reporter | ||
Comment 6•13 years ago
|
||
Please keep this bug comments scoped to anything only needed for review of the patch stack, thanks.
Reporter | ||
Comment 7•13 years ago
|
||
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.
Description
•