12334 tcp throughput benchmark and performance improvements by itamarst · Pull Request #12335 · twisted/twisted

itamarst · 2024-10-15T18:46:44Z

Scope and purpose

Fixes #12334

This includes:

A benchmark.
A couple of pretty straightforward optimizations.
~~One optimization I worry about a little.~~ (decided it was too risky)

…tWriting() in every reactor iteration.

src/twisted/internet/abstract.py

codspeed-hq · 2024-10-15T18:48:21Z

CodSpeed Performance Report

Merging #12335 will improve performances by 2.15%

_{Comparing 12334-tcp-throughput-benchmark (09d6383) with trunk (c3e5f02)}

🎉 Hooray! `pytest-codspeed` just leveled up to 3.0.0!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

Summary

⚡ 1 improvements
✅ 23 untouched benchmarks

🆕 1 new benchmarks

Benchmarks breakdown

	Benchmark	`trunk`	`12334-tcp-throughput-benchmark`	Change
⚡	`test_deferred_chained_already_fired`	64.8 µs	63.4 µs	+2.15%
🆕	`test_tcp_throughput`	N/A	5.1 ms	N/A

itamarst · 2024-10-15T18:58:16Z

codspeed claims faster benchmark for test_lineReceived and I am confused. This seems like a lie, none of my changes should affect existing benchmarks.

adiroiban · 2024-10-16T08:27:12Z

benchmarks/test_tcp_throughput.py

+    server = ServerFactory()
+    server.protocol = Server
+    port = reactor.listenTCP(0, server)


I am not sure if we are benchmarking client-side or server-side or both here.

I was thinking that for the benchmark for client-side tests , we can make an exception and have a single server that is used by all the tests runs.

That is, create the server as a global/singleton/fixture outside of the benchmark run.

It's benchmarking both I guess?

The client send sends in 16KB chunks, the server-side likely responds in bigger chunks, and this also benchmarks reading... insofar as the goal isn't to measure speed of sending, but to measure speed of Twisted in these sort of scenarios, it seems OK as is?

It's ok to benchmark both.

I guess that we can refactor this in the furure.

I was thinking that we might want to have separate benchmark tests for data processing speed for:

client send

client receive

server send

server receive

The current test is good enought to server as a general regression test.

I was thinking that for example if we improve performance for client send, while server receive performance is worse... the tests might still be ok, as the overall tests speed are not changed

Now... this is unlikely to happen

src/twisted/internet/abstract.py

adiroiban · 2024-10-16T09:21:56Z

@itamarst the test_line_receiver test is flaky ... see this PR where it has +10.19% without any changes to the line receiver code.

#1645 (comment)

…g()/startWriting() in every reactor iteration." This reverts commit 67dcad2.

…-benchmark

adiroiban

Many thanks for working on this

I only left a comment...and I am not sure what is going on here

and why self.offset = 0 is only conditionally set

In the previous code self.offset = 0 is alwasy called

src/twisted/internet/abstract.py

adiroiban · 2024-10-17T11:46:14Z

benchmarks/test_tcp_throughput.py

+    server = ServerFactory()
+    server.protocol = Server
+    port = reactor.listenTCP(0, server)


It's ok to benchmark both.

I guess that we can refactor this in the furure.

I was thinking that we might want to have separate benchmark tests for data processing speed for:

client send

client receive

server send

server receive

The current test is good enought to server as a general regression test.

I was thinking that for example if we improve performance for client send, while server receive performance is worse... the tests might still be ok, as the overall tests speed are not changed

Now... this is unlikely to happen

adiroiban

Just removing it from the review queue

Thanks Itamar for looking into this

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

itamarst · 2024-10-28T15:55:56Z

I have fixed the bug (thanks for catching it!) and improved testing to ensure this critical code path has better test coverage.

itamarst · 2024-10-28T18:48:49Z

Will fix static checks later today.

…into 12334-tcp-throughput-benchmark

adiroiban

Thanks for the changes.

I did a very quick review of the latest changes and they look good.

I didn't had time to look into the details of the hypothesis tests, but hope all is ok.

I don't know when I will have more time to review this PR...so I think that is ok to have it merged.

Thanks again

pythonspeed added 6 commits October 15, 2024 13:46

A benchmark of TCP throughput.

202fa05

Simplify this code path.

e8e44d1

Another optimization.

7cab979

News fragment.

e400907

Lints.

d758045

Another optimization: for pull producers, don't do stopWriting()/star…

67dcad2

…tWriting() in every reactor iteration.

itamarst linked an issue Oct 15, 2024 that may be closed by this pull request

TCP throughput benchmark #12334

Closed

itamarst changed the title ~~12334 tcp throughput benchmark and performance improvement~~ 12334 tcp throughput benchmark and performance improvements Oct 15, 2024

itamarst commented Oct 15, 2024

View reviewed changes

src/twisted/internet/abstract.py Show resolved Hide resolved

Delete unused methods.

f01ca5b

itamarst marked this pull request as ready for review October 15, 2024 18:56

adiroiban reviewed Oct 16, 2024

View reviewed changes

src/twisted/internet/abstract.py Show resolved Hide resolved

adiroiban reviewed Oct 16, 2024

View reviewed changes

src/twisted/internet/abstract.py Outdated Show resolved Hide resolved

adiroiban reviewed Oct 16, 2024

View reviewed changes

src/twisted/internet/abstract.py Outdated Show resolved Hide resolved

pythonspeed added 2 commits October 16, 2024 10:40

Revert "Another optimization: for pull producers, don't do stopWritin…

5401a8a

…g()/startWriting() in every reactor iteration." This reverts commit 67dcad2.

Merge remote-tracking branch 'origin/trunk' into 12334-tcp-throughput…

19f74c0

…-benchmark

itamarst requested a review from adiroiban October 16, 2024 17:51

itamarst added the needs-review label Oct 16, 2024

adiroiban requested a review from a team October 16, 2024 18:42

adiroiban reviewed Oct 17, 2024

View reviewed changes

adiroiban requested a review from a team October 17, 2024 11:47

pythonspeed added 2 commits October 28, 2024 10:16

Improved documentation.

59351d9

Fix a bug caught in code review, and add a corresponding test.

b2b24a3

adiroiban requested changes Oct 28, 2024

View reviewed changes

chevah-robot added needs-changes and removed needs-review labels Oct 28, 2024

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

7c1f18f

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

itamarst requested a review from adiroiban October 28, 2024 15:54

chevah-robot added needs-review and removed needs-changes labels Oct 28, 2024

Merge branch 'trunk' into 12334-tcp-throughput-benchmark

adf09a2

pythonspeed added 2 commits October 29, 2024 13:41

Lints and improved type annotations

83f0e08

Merge remote-tracking branch 'origin/12334-tcp-throughput-benchmark' …

09d6383

…into 12334-tcp-throughput-benchmark

adiroiban approved these changes Oct 29, 2024

View reviewed changes

chevah-robot added needs-merge and removed needs-review labels Oct 29, 2024

itamarst merged commit e297634 into trunk Oct 30, 2024

itamarst deleted the 12334-tcp-throughput-benchmark branch October 30, 2024 14:15

Conversation

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

Uh oh!

Scope and purpose

Uh oh!

Uh oh!

codspeed-hq bot commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CodSpeed Performance Report

Merging #12335 will improve performances by 2.15%

🎉 Hooray! pytest-codspeed just leveled up to 3.0.0!

Summary

Benchmarks breakdown

Uh oh!

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

Uh oh!

Uh oh!

adiroiban Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

itamarst Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

itamarst Oct 16, 2024

Choose a reason for hiding this comment

Uh oh!

adiroiban Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adiroiban commented Oct 16, 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!

adiroiban Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

itamarst commented Oct 28, 2024

Uh oh!

itamarst commented Oct 28, 2024

Uh oh!

adiroiban left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

itamarst commented Oct 15, 2024 •

edited

Loading

codspeed-hq bot commented Oct 15, 2024 •

edited

Loading

🎉 Hooray! `pytest-codspeed` just leveled up to 3.0.0!

itamarst commented Oct 15, 2024 •

edited

Loading

adiroiban left a comment •

edited

Loading