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

Reduce OpenSSL::Buffering#do_write overhead #831

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

byroot
Copy link
Member

@byroot byroot commented Dec 21, 2024

[Bug #20972]

The rb_str_new_freeze was added in #452 to better handle concurrent use of a Socket, but SSL sockets can't be used concurrently AFAIK, so we might as well just error cleanly.

By using rb_str_locktmp we can ensure attempts at concurrent write will raise an error, be we avoid causing a copy of the bytes.

We also use the newer String#append_as_bytes method when available to save on some more copies.

class Buffer
class Buffer < String
def append_as_bytes(string)
if string.encoding == Encoding::BINARY || string.ascii_only?
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better to "<< string" and force encoding again to binary at the end? I'm not familiar with ascii_only? impl, but I suspect it traverses all codepoints, and in the worst case scenario, string.b will allocate a new string right?

ext/openssl/ossl_ssl.c Outdated Show resolved Hide resolved
ext/openssl/ossl_ssl.c Outdated Show resolved Hide resolved
@rhenium
Copy link
Member

rhenium commented Dec 21, 2024

SSL sockets can't be used concurrently AFAIK

This is correct. The bug that must be fixed in the original PR was the -1 return from #syswrite. In the PR I took the easier path, but modifying the String in another thread isn't supported.

if @sync or buffer_size > BLOCK_SIZE
nwrote = 0
begin
while nwrote < buffer_size do
begin
nwrote += syswrite(@wbuffer[nwrote, buffer_size - nwrote])
chunk = if nwrote > 0
@wbuffer.byteslice(nwrote..-1)
Copy link

@luke-gru luke-gru Dec 22, 2024

Choose a reason for hiding this comment

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

It doesn't matter much, but we might as well use integer arguments instead of a range to save on the range allocation. BTW thanks for putting me as co-author @byroot, I appreciate it 😄 Edit: nevermind, it's just a putobject call for the range 😄

@byroot byroot force-pushed the reduce-buffering-do-write-overhead branch 3 times, most recently from 0980748 to c0b9408 Compare December 27, 2024 13:14
@byroot
Copy link
Member Author

byroot commented Dec 27, 2024

Interestingly, it seems TruffleRuby doesn't like locking of frozen strings:


11) Error: test_write_nonblock_with_buffered_data_no_exceptions(OpenSSL::TestPairLowlevelSocket): RuntimeError: temporal locking immutable string
string.c:102:in `rb_str_locktmp'
string.c:102:in `rb_str_locktmp'

@byroot byroot force-pushed the reduce-buffering-do-write-overhead branch from c0b9408 to 8db1bcd Compare December 27, 2024 13:38
@byroot byroot force-pushed the reduce-buffering-do-write-overhead branch from 8db1bcd to 4e8df0f Compare December 27, 2024 13:42
@byroot byroot force-pushed the reduce-buffering-do-write-overhead branch from 4e8df0f to 910e6ab Compare January 7, 2025 10:29
byroot added 2 commits January 7, 2025 11:34
[Bug #20972]

The `rb_str_new_freeze` was added in ruby#452
to better handle concurrent use of a Socket, but SSL sockets can't be used
concurrently AFAIK, so we might as well just error cleanly.

By using `rb_str_locktmp` we can ensure attempts at concurrent write
will raise an error, be we avoid causing a copy of the bytes.

We also use the newer `String#append_as_bytes` method when available
to save on some more copies.

Co-Authored-By: [email protected]
@byroot byroot force-pushed the reduce-buffering-do-write-overhead branch from 2879f43 to 28f2901 Compare January 7, 2025 10:34
@rhenium rhenium merged commit a3e0c16 into ruby:master Jan 14, 2025
63 checks passed
@rhenium
Copy link
Member

rhenium commented Jan 14, 2025

Looks good to me, thank you!

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

Successfully merging this pull request may close these issues.

5 participants