Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6477 defect closed fixed (fixed)

twisted.protocols.tls uses quadratic complexity buffer management when writing

Reported by: Peter Owned by: Jean-Paul Calderone
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/ssl-largewrite-throughput-6477
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

Hi,

I'm new to this code. I'm sending 40 MB via TLS, and _write(self, bytes) in twisted/protocols/tls.py uses

leftToSend = leftToSend[sent:]

which is extremely slow since 16kb is written each time. It takes half a minute to send this 40 MB and uses 100% of one of the cores.

I modified the code in order to send only bytes[alreadySent:alreadySent+16384], where alreadySent += sent. This solved the issue. Sorry for not attaching diff, I don't have time, I just wanted to let you guys know.

Peter

Change History (8)

comment:1 Changed 5 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/ssl-largewrite-throughput-6477

(In [38274]) Branching to 'ssl-largewrite-throughput-6477'

comment:2 Changed 5 years ago by Jean-Paul Calderone

(In [38275]) Walk a buffer through the input instead of repeatedly slicing the unwritten part off the end

refs #6477

comment:3 Changed 5 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned
Summary: Sending large file via tls is slowtwisted.protocols.tls uses quadratic complexity buffer management when writing

comment:4 Changed 5 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

I think I dealt with this in the branch.

Here are the build results. Unfortunately the slave for the Python 3 builder is MIA. I'm not comfortable merging until that comes back and shows us this works there as well. Perhaps it will by the time a reviewer gets here, though.

I also added two new benchmarks to lp:twisted-benchmarks, currently proposed for merging on Launchpad. It'd be nice to merge and deploy those changes first, so we can see things actually get faster.

For a taste, on my 2.2GHz Celeron laptop, I observed no change in ssl_throughput and a 30% speedup in ssl_throughput_bigwrite and ssl_throughput_smallwrite.

comment:5 Changed 5 years ago by Jean-Paul Calderone

Actually, scratch the bit about ssl_throughput_smallwrite - I don't think it changes much either.

comment:6 Changed 5 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

It looks good to me. I'd just mention that no test fail if I replace alreadySend by 0 in the except WantReadError clause. It also makes me notice that we do a useless copy here when alreadySend is 0 (probably very minor).

comment:7 Changed 5 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [38313]) Merge ssl-largewrite-throughput-6477

Author: exarkun Reviewer: therve Fixes: #6477

In twisted.protocols.tls, replace some O(N2) complexity buffer handling code with some code of somewhat lower complexity. This primarily helps improve performance when a single call to write is passed a very large string (the further above 2 16, the bigger impact this will likely have).

comment:8 Changed 5 years ago by Jean-Paul Calderone

I'd just mention that no test fail if I replace alreadySend by 0 in the except WantReadError clause.

Filed #6500 for this.

It also makes me notice that we do a useless copy here when alreadySend is 0 (probably very minor).

CPython does this micro-optimization for us - s[0:] is s. I assume PyPy is smart enough not to waste time doing a copy for this operation, too.

Note: See TracTickets for help on using tickets.