659760 - (pipelining-review) Review patches for HTTP pipelining

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

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)]

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)]

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)]

(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] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

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]]

(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)]

Please keep this bug comments scoped to anything only needed for review of the patch stack, thanks.

Comment 7

[Honza Bambas (:mayhemer)]

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.