search

secondlife / viewer

🖥️ Second Life's official client
GNU Lesser General Public License v2.1
212 stars 53 forks source link

Disable HTTP Pipelining #3035

Open AiraYumi opened 2 weeks ago

AiraYumi commented 2 weeks ago

What you need to do before you do this:

monty-linden commented 6 days ago

@akleshchev tapped me on the shoulder and suggested I comment.

Quick question: did you measure the before-and-after performance effects of this change?

Longer response: a great deal of effort went into parameter tuning years ago when the http system was changed, a CDN was introduced, and pipelining was implemented. Throughput and latency were greatly affected by parameter changes and the original settings (a subset of current ones) were a pretty good tradeoff of various factors. Are these tuned correctly today? Perhaps not but I have a fairly strong hunch eliminating pipelining without a compensating change in, say, concurrency, is likely to have a negative performance impact.

And at the moment, we're a bit gun-shy about releases with negative performance impacts.

Fortunately, the tools for a better tomorrow exist and have for more than a decade! The example code in llcorehttp also happens to be a performance testing scaffold. Some specially instrumented viewers can generate real-world asset fetch lists (asset, range, etc.) which feed the tool and allow parameter tuning anywhere in the world. And completely isolated from the full viewer environment and its repeatability problems.

Yeah, we know HTTP/1.1 pipelining needs to go along with the freeze on the libcurl package. The compensating feature for that should probably be HTTP/2. That, in turn, will add the additional cost of crypto to the asset fetching problem but I hope to see it perform comparably without the reliability problems of pipelining. Just a matter of coding and a lot of testing.

AiraYumi commented 6 days ago

Thank you @monty-linden .

We are currently checking the viewer's performance after making this change.

If the CDN you are currently using supports HTTP/2, even if you disable HTTP Pileline, you should be able to minimize the impact on performance by also enabling HTTP/2.

Fortunately, libcurl allows you to enable HTTP/2 communication by simply adding a single option.

We will check the performance.

AiraYumi commented 6 days ago

I found that libcurl can automatically attempt multiplexing when http/2 is enabled. I don't know why this wasn't enabled. Perhaps an asset server issue?

monty-linden commented 6 days ago

The larger concern I was hinting at is that libcurl isn't just a library, it is part of infrastructure which is larger than viewer development. Targets need to be compatible and the CDN hasn't been setup yet for http/2 (although it can be used experimentally). Changes in libcurl usually come with changes in crypto. Viewer crypto needs to negotiate with server crypto which isn't always compatible. Server crypto changes can then fail to negotiate with scripters' remote https clients and servers and everyone needs testing and conversion time before switchover. The viewer itself needs debug flags or full settings to disable http/2 when someone can't use it for some reason. And the frozen version of libcurl has an http/2 implementation that is probably unusable.

What looks like flipping two booleans in the viewer code is actually a large infrastructure upgrade project. But that can start with understanding viewer impact. I.e. what happens across the world when pipelining is disabled?

AiraYumi commented 6 days ago

I was looking at the code, and I think that even though mPipelined is set to true, HTTP pipelining is not actually working. This is because, in the C++ standard, true is changed to 1 and false is changed to 0 when compared with numeric types, but in the code, HTTP pipelining is enabled when the value is greater than 1.

Standard conversions (Integral promotions) https://timsong-cpp.github.io/cppwp/n4659/conv.prom#6

Quinn-Elara commented 5 days ago

@akleshchev tapped me on the shoulder and suggested I comment.

Quick question: did you measure the before-and-after performance effects of this change?

Longer response: a great deal of effort went into parameter tuning years ago when the http system was changed, a CDN was introduced, and pipelining was implemented. Throughput and latency were greatly affected by parameter changes and the original settings (a subset of current ones) were a pretty good tradeoff of various factors. Are these tuned correctly today? Perhaps not but I have a fairly strong hunch eliminating pipelining without a compensating change in, say, concurrency, is likely to have a negative performance impact.

And at the moment, we're a bit gun-shy about releases with negative performance impacts.

Fortunately, the tools for a better tomorrow exist and have for more than a decade! The example code in llcorehttp also happens to be a performance testing scaffold. Some specially instrumented viewers can generate real-world asset fetch lists (asset, range, etc.) which feed the tool and allow parameter tuning anywhere in the world. And completely isolated from the full viewer environment and its repeatability problems.

Yeah, we know HTTP/1.1 pipelining needs to go along with the freeze on the libcurl package. The compensating feature for that should probably be HTTP/2. That, in turn, will add the additional cost of crypto to the asset fetching problem but I hope to see it perform comparably without the reliability problems of pipelining. Just a matter of coding and a lot of testing.

The problem with this is that while yes, you can disable pipelining and yes, chances are the performance impact is negative, the CDN itself has HTTP/2 disabled (manually verified by calling cURL from terminal and forcing the use of HTTP/2, which then states that the remote server does not support HTTP/2 ), so any chance to test and fix performance regressions is not possible.

I'd suggest at least enabling HTTP/2 on the CDN side for the beta grid (aditi) so that performance testing and fixes for HTTP/2 can be worked on, rather than the current situation of chicken-and-egg where the viewer work can't go ahead without serverside support, and the serverside switch won't be flipped unless the viewer has support and has been tested with satisfactory performance.

monty-linden commented 5 days ago

and the serverside switch won't be flipped unless the viewer has support and has been tested with satisfactory performance.

No such condition exists. The server side is more than flipping a switch (hints of which can be seen in cert and cipher negotiation if you're looking at curl details). I've been trying to trick some internal viewer dev to take on http/2 without success. But if someone is ready to give it a try, we can look at pushing those infra changes along.

As for the PR: I'll block with changes requested for now. The changes will be gated by Linden moving the grid infra forward to support http/2 dev work.

AiraYumi commented 5 days ago

We've created pull request #3101 to enable HTTP/2 support on the viewer side. However, work is still needed.

We've found that we're not yet ready to disable HTTP pipelining at this stage, so we'll stop work on this pull request until the server side is ready.

brad-linden commented 1 day ago

seems like this is the kind of thing that should be discussed in issues to reach consensus on a plan before we start working on PRs