Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#12011 Speed up small TLS writes #12090

Conversation

itamarst
Copy link
Contributor

Fixes #12011.

Benchmark results with Python 3.11:

trunk:

$ time python tls_benchmark.py small

real    0m2.425s
user    0m2.302s
sys     0m0.069s

This branch:

$ time python tls_benchmark.py small

real    0m1.705s
user    0m1.562s
sys     0m0.088s

Benchmark:

import sys

from twisted.internet.protocol import Protocol
from twisted.internet.ssl import CertificateOptions, PrivateCertificate
from twisted.protocols.loopback import loopbackTCP
from twisted.internet.task import react
from twisted.internet import reactor

class Client(Protocol):
    def connectionMade(self):
        self.count = 10_000
        self.transport.startTLS(CertificateOptions(verify=False, requireCertificate=False))
        assert sys.argv[1] in ("small", "big")
        self.next_iteration()

    def next_iteration(self):
        self.count -= 1
        if sys.argv[1] == "small":
            for i in range(1_000):
                self.transport.write(b"X")
        else:
            self.transport.write(b"X" * 1000)

        if self.count == 0:
            self.transport.loseConnection()
        else:
            reactor.callLater(0, self.next_iteration)


class Server(Protocol):
    received = 0

    def connectionMade(self):
        with open("server.pem", "rb") as f:
            options = PrivateCertificate.loadPEM(f.read()).options()
        self.transport.startTLS(options)

    def dataReceived(self, data):
        self.received += len(data)

    def connectionLost(self, reason):
        assert self.received == 10_000_000, self.received


assert len(sys.argv) == 2
react(lambda reactor: loopbackTCP(Server(), Client(), noisy=True))

@adiroiban
Copy link
Member

Hi Itamar, Thanks for looking into this. Is this PR ready for review? Regards

@itamarst
Copy link
Contributor Author

Yes, it's ready for review.

@glyph glyph requested review from a team and removed request for adiroiban January 27, 2024 22:05
Copy link
Member

@adiroiban adiroiban left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Thanks. Only minor comments.

# This is kinda ugly, but speeds things up a lot in a hot path with
# lots of small TLS writes. May become unnecessary in Python 3.13 or
# later if JIT and/or inlining becomes a thing.
self.write = self._aggregator.write # type: ignore[method-assign]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not pretty, but I think is the best option.

I think that, in the end, having the _AggregateSmallWrites code using composition is nicer.

@@ -687,25 +687,6 @@ def connectionMade(self):

return self.writeBeforeHandshakeTest(SimpleSendingProtocol, data)

def test_writeUnicodeRaisesTypeError(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe leave this test as a reminder that passing bytes should never be supported again.

But I am also ok to have this removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is it doesn't fail in a useful way now.

Copy link
Member

@adiroiban adiroiban Jan 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the important part is to make sure that it fails... in any way. Rather than ending up with a case in which the data is ignored... or something like that.

So even if the error is not explicit. I think that it still important to make sure that it fails.

The previous explicit error was a huge help for my in the past... but I am ok to have something less helful, in the name of performance.

I hope in the future mypy can help here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I restored the test.

src/twisted/newsfragments/12011.bugfix Outdated Show resolved Hide resolved
Co-authored-by: Adi Roiban <adiroiban@gmail.com>
pythonspeed and others added 5 commits January 29, 2024 10:58
…ssert-that-each-written-chunk-of-a-data-is-not-a-str' into 12011-performance-issue-in-tls-assert-that-each-written-chunk-of-a-data-is-not-a-str
@itamarst itamarst merged commit 12c8a5c into trunk Jan 29, 2024
23 checks passed
@itamarst itamarst deleted the 12011-performance-issue-in-tls-assert-that-each-written-chunk-of-a-data-is-not-a-str branch January 29, 2024 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Performance issue in TLS: assert that each written chunk of a data is not a str
4 participants