Last Comment Bug 659760 - (pipelining-review) Review patches for HTTP pipelining
(pipelining-review)
: Review patches for HTTP pipelining
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: All All
: -- normal with 3 votes (vote)
: mozilla11
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 615342 232030 447866 597684 597706 599164 602518 603505 603506 603508 603512 603514 631801 632496 665885
Blocks: 603503
  Show dependency treegraph
 
Reported: 2011-05-25 14:12 PDT by Honza Bambas (:mayhemer)
Modified: 2016-02-08 09:40 PST (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Honza Bambas (:mayhemer) 2011-05-25 14:12:53 PDT
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?
Comment 1 Honza Bambas (:mayhemer) 2011-06-17 14:23:19 PDT
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
Comment 2 Honza Bambas (:mayhemer) 2011-12-06 11:12:14 PST
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.
Comment 3 Honza Bambas (:mayhemer) 2011-12-06 11:14:17 PST
(In reply to Honza Bambas (:mayhemer) from comment #2)
>               * bug 540108 Provide basic pipeline ban API
bug 665094 is the one here...
Comment 4 Virtual_ManPL [:Virtual] - (ni? me) 2011-12-08 07:03:37 PST
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 Patrick McManus [:mcmanus] 2011-12-08 07:13:07 PST
(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).
Comment 6 Honza Bambas (:mayhemer) 2011-12-08 08:34:12 PST
Please keep this bug comments scoped to anything only needed for review of the patch stack, thanks.
Comment 7 Honza Bambas (:mayhemer) 2012-02-09 14:35:33 PST
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.

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