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

Poor performance on Connection.recv() with large values of bufsiz. #577

Closed
Lukasa opened this Issue Nov 27, 2016 · 2 comments

Comments

Projects
None yet
1 participant
@Lukasa
Copy link
Member

Lukasa commented Nov 27, 2016

Discovered while investigating requests/requests#3729.

When allocating a buffer using CFFI's new method (as Connection.recv does via _ffi.new("char[]", bufsiz), CFFI kindly zeroes that buffer for us. That means this call has a performance cost that is linear in bufsiz. This can lead to nasty negative behaviour if really large values of bufsiz are used, e.g. 1GB.

We don't need a zeroed buffer for this: SSL_read returns a length, so we know exactly how many bytes to copy out. Unfortunately though, CFFI doesn't provide any method to obtain a non-zeroed buffer, at least not as far as I can see.

Instead, a coping method might be to silently clamp the maximum value of bufsiz to the return value of self._socket.getsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF). This is the size of the socket receive buffer, and therefore represents the maximum amount of data the kernel can possibly return in one recv call. On my OS X machine, this is ~129 kB by default. An alternative option is to clamp to 16kB, the maximum TLS record size, or to clamp to the nearest multiple of 16kB near SO_RCVBUF. Either of these will greatly shrink the allocations. Finally, we can just pick an arbitrary value: a few hundred KB is probably safe.

I should note, however, that this coping method still imposes a substantial cost when downloading data that is not incurred by the standard library's ssl module. Specifically, CFFI will require that we zero at least as many bytes as the size of the download. This is all wasted effort, and for multi-gigabyte files represents really quite a substantial waste of time. It would be best to do both of these things: to shrink the size of our allocations and to try to avoid zeroing data when we don't need to.

@Lukasa Lukasa changed the title Poor performance on Connection.recv() with large values of amt. Poor performance on Connection.recv() with large values of bufsiz. Nov 27, 2016

@Lukasa

This comment has been minimized.

Copy link
Member

Lukasa commented Nov 27, 2016

So, Armin has pointed out that this actually already exists: ffi.new_allocator(should_clear_after_alloc=False)("char[]", bufsiz) should be a solution to the zeroing out of memory. I'm going to investigate it now.

@Lukasa

This comment has been minimized.

Copy link
Member

Lukasa commented Nov 27, 2016

Excellently, that does work. I'll start working on a PR for this tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment