#12108 Speed up the twisted.web HTTP client by itamarst · Pull Request #12109 · twisted/twisted

itamarst · 2024-03-06T16:00:19Z

Scope and purpose

Why this is faster:

Minor reductions in general effort.
Only calculate Failure.parents on demand, since it's expensive and often not used.
Failure.__init__ does a huge amount of unnecessary work for simple cases, so avoided it with alternative constructor. The constructor results in different pickling behavior, because of subclassing BaseException it seems!!!! See https://gist.github.com/itamarst/4e984bddef23a96619b37bfb929e5b24

I run my benchmark script with hyperfine to reduce noise from dictionary ordering randomness and the like.

Before:

$ hyperfine --warmup 1 "python http_client_benchmark.py"
Benchmark 1: python http_client_benchmark.py
  Time (mean ± σ):      3.992 s ±  0.056 s    [User: 3.979 s, System: 0.012 s]
  Range (min … max):    3.917 s …  4.118 s    10 runs

After:

$ hyperfine --warmup 1 "python http_client_benchmark.py"
Benchmark 1: python http_client_benchmark.py
  Time (mean ± σ):      3.266 s ±  0.032 s    [User: 3.253 s, System: 0.012 s]
  Range (min … max):    3.222 s …  3.336 s    10 runs

ORIGINAL APPROACH, NOW OBSOLETE

Important: This relies on Failure objects being effectively immutable, which I think is the case but should be something to consider when reviewing.

I run my benchmark script with hyperfine to reduce noise from dictionary ordering randomness and the like.

Before:

$ hyperfine --warmup 1 "python http_client_benchmark.py"
Benchmark 1: python http_client_benchmark.py
  Time (mean ± σ):      3.992 s ±  0.056 s    [User: 3.979 s, System: 0.012 s]
  Range (min … max):    3.917 s …  4.118 s    10 runs

After:

$ hyperfine --warmup 1 "python http_client_benchmark.py"
Benchmark 1: python http_client_benchmark.py
  Time (mean ± σ):      3.022 s ±  0.025 s    [User: 3.005 s, System: 0.016 s]
  Range (min … max):    2.982 s …  3.051 s    10 runs

Test script:

from twisted.web._newclient import HTTP11ClientProtocol, Request
from twisted.internet.testing import StringTransport
from twisted.web.http_headers import Headers
import time

RESPONSE = """HTTP/1.1 200 OK
Host: blah
Foo: bar
Gaz: baz
Content-length: 3

abc""".replace("\n", "\r\n").encode("utf-8")

start = time.time()
for i in range(100_000):
    protocol = HTTP11ClientProtocol()
    protocol.makeConnection(StringTransport())
    request = Request(b"GET", b"/foo/bar", Headers({b"Host": [b"example.com"]}), None)
    d = protocol.request(request)
    protocol.dataReceived(RESPONSE)
    l = []
    d.addCallback(l.append)
    assert l
print("requests/sec:", 100_000 / (time.time() - start))

Follow-up PRs will:

Optimize http_headers.py, which does a lot of repetitive and unnecessary work.
Possibly split up the identity transfer encoding into two variants, one with a fixed length and one without.

Contributor Checklist:

This process applies to all pull requests - no matter how small.
Have a look at our developer documentation before submitting your Pull Request.

Below is a non-exhaustive list (as a reminder):

The title of the PR should describe the changes and starts with the associated issue number, like “#9782 Remove twisted.news. #1234 Brief description”.
A release notes news fragment file was create in src/twisted/newsfragments/ (see: Release notes fragments docs.)
The automated tests were updated.
Once all checks are green, request a review by leaving a comment that contains exactly the string please review.
Our bot will trigger the review process, by applying the pending review label
and requesting a review from the Twisted dev team.

for more information, see https://pre-commit.ci

…lient' into 12108-speed-up-twistedweb-http-client

itamarst · 2024-03-06T16:15:01Z

please review

adiroiban

Many thanks for the changes.

For now, I just left this is a comment, and put back the team for review.

Happy to see performance improvements in the HTTP client.

I left a few comments inline.

As you commented in the description of the PR, my biggest concern is related to mutating the failure instance.

I remember seing instances in which a failure is mutated, but I don't remember the exact code or the ticket/issue number.

I think that for the purpose of the current code, it's ok to this change.

I expect that at least some downstream application/library that relies on Twisted is mutating failures

I think I have some places in my applications in which I mutate Failure.value. This was done for convenience...but I think the right thing to do is to have someting like newFailure = oldFailure.copy(Exception('new exception')) in which the traceback is kept.

I am doing this type of things mostly in high level application code.
The error messsage text generated by Twisted is not good enought for end users... but I still want to keep the traceback for troubleshoting

Maybe we can create a new FrozenFailure class and use it here.
The FrozenFailure will raise an error is someone tries to mutate it.

src/twisted/web/_newclient.py

itamarst · 2024-03-06T23:55:49Z

It's not clear to me there is a traceback in this case, since it's directly creating exceptions. Will validate tomorrow.

adiroiban · 2024-03-07T13:30:51Z

It's not clear to me there is a traceback in this case, since it's directly creating exceptions. Will validate tomorrow.

You are right.
There is no traceback here, so traceback is not an issue.

Still, maybe it's a good idea to have different text messages, depending on the place from where this failure is returned.

itamarst · 2024-03-11T18:34:15Z

So, the main issue is mutable Failure. FrozenFailure is certainly possible, but... it'd have to make the exception frozen somehow, when it's not clear how. So a generic version is hard. And any mutable exception attributes can be mutated even if __setattr__ is disabled... so a generic version seems difficult.

On the other hand, here it's just ResponseDone and ConnectionDone which don't have mutable attributes, really, and oughtn't be mutated. So might be doable in this case.

itamarst · 2024-03-11T20:58:20Z

Actually, there's a number of places that just assume immutable exceptions already, e.g. the CONNECTION_DONE constant, so maybe just read-only Failure is sufficient.

itamarst · 2024-03-11T21:11:22Z

OK, added a Failure subclass.

glyph · 2024-03-11T22:10:30Z

OK, added a Failure subclass.

This is probably fine, but thinking about it does send a chill down my spine.

I don't think that an immutable failure subclass is actually substitutable, or thereby compliant with the letter of the compatibility policy, as it stands. I'd kinda rather make all Failures be immutable if that is practical, and initially enforce it at type-time, which is not covered by the compatibility policy?

exarkun · 2024-03-12T01:21:16Z

OK, added a Failure subclass.

This is probably fine, but thinking about it does send a chill down my spine.

cough CopyableFailure cough

I don't think that an immutable failure subclass is actually substitutable, or thereby compliant with the letter of the compatibility policy, as it stands. I'd kinda rather make all Failures be immutable if that is practical, and initially enforce it at type-time, which is not covered by the compatibility policy?

It's certainly not. I think only enforcing it at type-time is probably good enough. There will never be a 100% immutable Python object. If the type checker tells someone they're not allowed to change something and they go ahead and run code that changes it anyway, that seems like a choice that's up to them.

Ideally, there'd be a generic like typing.Readonly - as in, Readonly[Failure]. I don't think there is, but a ReadonlyFailure protocol that exposes all of Failure's public attributes, but only for reading is still simple, just more typing. Then I believe the definition of freeze is:

    def freeze(self) -> ReadonlyFailure: return self

exarkun · 2024-03-12T01:21:55Z

(This PR also seems like a weird place to be solving this 24 year old design mistake)

itamarst · 2024-03-12T01:39:15Z

python/typing#1123 has an example of how to define a Protocol with read-only attributes. But actually these days you can just use Final, which is simpler.

itamarst · 2024-03-12T01:40:16Z

Given the Failure object is going to end up in places, e.g. Protocol.connectionLost for a body protocol of a Response, using typing to enforce read-only-ness might end up going pretty far in terms of changing the API.

glyph · 2024-03-12T03:47:27Z

cough CopyableFailure cough

Okay, perhaps a comparable amount of chills, but it is a proper Liskov substitute ;)

glyph · 2024-03-12T03:51:51Z

Then I believe the definition of freeze is:

My suggestion would be to just have Failure itself be frozen by default, because:

Mutating a failure is a pretty weird operation that I would guess rarely happens?
Any code doing the mutating would continue to work at runtime, unless it mutates one of these shared instances (but what mutation would such code be making, I wonder?) in which case behavior would change, but not in a technically incompatible way.
Any code doing the mutating would become a type error, which seems like a reasonable way for users to catch it and fix things.

If my first assumption is wrong (this would not affect much code) then we should probably do another approach. Although then I'd really want to know what the common failure-mutation patterns are.

adiroiban

Thanks for the changes.

I only have a minor comment regarding the release notes wording.

Based on the latest discussions, I think that we can merge this PR as it is.

Also, I am also ok with merging this code with a standard Failure, just like it was in the first review request.

I have approved this PR, but maybe wait one day to receive more feedback from Jean-Paul and Glyph.

(This PR also seems like a weird place to be solving this 24 year old design mistake)

We don't have to make big API changes in this PR.
I think it is still usefull to discuss this issue.

I was not able to find an existing ticket/GitHub Issue discussing the read only design for Failure.

I guess we should create a new one now :)

I'd kinda rather make all Failures be immutable if that is practical,

I think that such a change should be done in a separate PR.

So my suggestion for this PR:

Create a new GitHub issue dedicated to the Failure read-only design (or find the existing one, if such an issue was already raised)
Commit this PR using using _FrozenFailure and add a note to remove it once the Failure read-only changes are made

Thanks again!

src/twisted/web/newsfragments/12108.feature

itamarst · 2024-03-12T13:12:30Z

There is one existing Failure API that mutates it, https://docs.twisted.org/en/stable/api/twisted.python.failure.Failure.html#cleanFailure

…lient' into 12108-speed-up-twistedweb-http-client

itamarst · 2024-03-14T12:54:48Z

please review

itamarst · 2024-03-14T12:55:13Z

MyPy failures don't appear to have anything to do with this branch...

for more information, see https://pre-commit.ci

glyph · 2024-03-14T20:30:06Z

MyPy failures don't appear to have anything to do with this branch...

Confirmed, fresh trunk checkout gives similar errors here. Investigating…

glyph · 2024-03-14T20:30:57Z

MyPy failures don't appear to have anything to do with this branch...

Confirmed, fresh trunk checkout gives similar errors here. Investigating…

filed #12113 to track and discuss

glyph · 2024-03-14T20:52:09Z

@itamarst an attempted fix is here #12115

glyph · 2024-03-14T21:01:11Z

OK, fix attempt seems to have worked, hopefully that can unblock this

glyph · 2024-03-15T00:05:42Z

Merging in the approved fix, so maybe we can get a successful run.

glyph

just some coding-standard compliance suggestions

src/twisted/test/test_failure.py

src/twisted/python/failure.py

src/twisted/test/test_failure.py

src/twisted/web/_newclient.py

adiroiban · 2024-03-15T09:16:16Z

src/twisted/logger/_json.py

    typeInfo = failureDict["type"]
    failureDict["type"] = type(typeInfo["__name__"], (), typeInfo)
-    f.__dict__ = failureDict
+    f.__setstate__(failureDict)


Is this API used ?

In src/twisted/logger/__init__.py I can see only these as public API

from ._json import ( eventAsJSON, eventFromJSON, jsonFileLogObserver, eventsFromJSONLogFile, )

and I don't see any test for this code.

The tests were failing without this, so I think so.

adiroiban · 2024-03-15T09:19:23Z

src/twisted/python/failure.py

            tb = tb.tb_next
+
+    @property
+    def parents(self):


I know that in trunk we alrady have Failure.parents as public API and without documentation.

Can we add a few words about Failure.parents in this PR?

The documentation should go in Failure.__doc__ and not here... but I can't leave a comment there.

It's an implementation detail, it's better if people don't use it. The fact it's public is a historic quirk...

Thanks for the info.

If this should be private, then maybe we need a followup issue to mark it private and raise a deprecation warning for any public usage of the API ?

src/twisted/newsfragments/12112.bugfix

src/twisted/test/test_failure.py

Co-authored-by: Glyph <glyph@twistedmatrix.com>

pythonspeed added 4 commits March 6, 2024 08:50

Don't create a new exception and Failure on every request.

33793eb

maybeDeferred is slow.

4f94806

Don't create a new Failure for every response.

a742047

newsfragment

6ed02b5

itamarst linked an issue Mar 6, 2024 that may be closed by this pull request

Speed up twisted.web HTTP client #12108

Closed

pre-commit-ci bot and others added 3 commits March 6, 2024 16:01

[pre-commit.ci] auto fixes from pre-commit.com hooks

e9f0a11

for more information, see https://pre-commit.ci

Another minor optimization.

866db5a

Merge remote-tracking branch 'origin/12108-speed-up-twistedweb-http-c…

9fa2b32

…lient' into 12108-speed-up-twistedweb-http-client

chevah-robot added the needs-review label Mar 6, 2024

chevah-robot requested a review from a team March 6, 2024 16:15

adiroiban reviewed Mar 6, 2024

View reviewed changes

src/twisted/web/_newclient.py Show resolved Hide resolved

src/twisted/web/_newclient.py Show resolved Hide resolved

src/twisted/web/_newclient.py Outdated Show resolved Hide resolved

src/twisted/web/_newclient.py Outdated Show resolved Hide resolved

adiroiban requested a review from a team March 6, 2024 19:58

Add a frozen Failure subclass.

ccf9ba6

adiroiban approved these changes Mar 12, 2024

View reviewed changes

src/twisted/web/newsfragments/12108.feature Outdated Show resolved Hide resolved

chevah-robot added needs-merge and removed needs-review labels Mar 12, 2024

pythonspeed added 3 commits March 14, 2024 08:48

Fix lints and mypy.

a9d86fb

Merge remote-tracking branch 'origin/12108-speed-up-twistedweb-http-c…

bdb0a5c

…lient' into 12108-speed-up-twistedweb-http-client

Move import to correct location.

5dc257b

itamarst marked this pull request as ready for review March 14, 2024 12:54

chevah-robot added the needs-review label Mar 14, 2024

chevah-robot requested a review from a team March 14, 2024 12:54

itamarst requested a review from adiroiban March 14, 2024 12:54

[pre-commit.ci] auto fixes from pre-commit.com hooks

2509a9a

for more information, see https://pre-commit.ci

Merge branch 'trunk' into 12108-speed-up-twistedweb-http-client

1ea4db4

glyph reviewed Mar 15, 2024

View reviewed changes

adiroiban reviewed Mar 15, 2024

View reviewed changes

src/twisted/newsfragments/12112.bugfix Outdated Show resolved Hide resolved

adiroiban reviewed Mar 15, 2024

View reviewed changes

src/twisted/test/test_failure.py Outdated Show resolved Hide resolved

itamarst and others added 3 commits March 15, 2024 09:38

Apply suggestions from code review

ed22e47

Co-authored-by: Glyph <glyph@twistedmatrix.com>

Match standard style

8b7503a

Delete unnecessary code

7682222

itamarst merged commit 087afb8 into trunk Mar 15, 2024

itamarst deleted the 12108-speed-up-twistedweb-http-client branch March 15, 2024 14:14

This was referenced Aug 5, 2024

Release 24.7.0 #12271

Closed

Regression since 2024.3.0: CopiedFailure relies on parents being settable #12279

Closed

#12279 Fix regression in CopiedFailure #12280

Merged

adiroiban mentioned this pull request May 15, 2025

Sending CopiedFailure between old and new version of Twisted crashes since 24.7.0 #12452

Open

Conversation

itamarst commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scope and purpose

ORIGINAL APPROACH, NOW OBSOLETE

Contributor Checklist:

Uh oh!

itamarst commented Mar 6, 2024

Uh oh!

adiroiban left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

itamarst commented Mar 6, 2024

Uh oh!

adiroiban commented Mar 7, 2024

Uh oh!

itamarst commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarst commented Mar 11, 2024

Uh oh!

itamarst commented Mar 11, 2024

Uh oh!

glyph commented Mar 11, 2024

Uh oh!

exarkun commented Mar 12, 2024

Uh oh!

exarkun commented Mar 12, 2024

Uh oh!

itamarst commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

itamarst commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glyph commented Mar 12, 2024

Uh oh!

glyph commented Mar 12, 2024

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

itamarst commented Mar 12, 2024

Uh oh!

itamarst commented Mar 14, 2024

Uh oh!

itamarst commented Mar 14, 2024

Uh oh!

glyph commented Mar 14, 2024

Uh oh!

glyph commented Mar 14, 2024

Uh oh!

glyph commented Mar 14, 2024

Uh oh!

glyph commented Mar 14, 2024

Uh oh!

glyph commented Mar 15, 2024

Uh oh!

glyph left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adiroiban Mar 15, 2024

Choose a reason for hiding this comment

Uh oh!

itamarst commented Mar 6, 2024 •

edited

Loading

adiroiban left a comment •

edited

Loading

itamarst commented Mar 11, 2024 •

edited

Loading

itamarst commented Mar 12, 2024 •

edited

Loading

itamarst commented Mar 12, 2024 •

edited

Loading