Reduce OpenSSL::Buffering#do_write overhead #831
Conversation
lib/openssl/buffering.rb
Outdated
| class Buffer | ||
| class Buffer < String | ||
| def append_as_bytes(string) | ||
| if string.encoding == Encoding::BINARY || string.ascii_only? |
There was a problem hiding this comment.
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?
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. |
lib/openssl/buffering.rb
Outdated
| 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) |
There was a problem hiding this comment.
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
force-pushed
the
reduce-buffering-do-write-overhead
branch
3 times, most recently
from
0980748 to
c0b9408
Compare
|
Interestingly, it seems TruffleRuby doesn't like locking of frozen strings: |
byroot
force-pushed
the
reduce-buffering-do-write-overhead
branch
from
c0b9408 to
8db1bcd
Compare
byroot
force-pushed
the
reduce-buffering-do-write-overhead
branch
from
8db1bcd to
4e8df0f
Compare
byroot
force-pushed
the
reduce-buffering-do-write-overhead
branch
from
4e8df0f to
910e6ab
Compare
[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
force-pushed
the
reduce-buffering-do-write-overhead
branch
from
2879f43 to
28f2901
Compare
|
Looks good to me, thank you! |
[Bug #20972]
The
rb_str_new_freezewas 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_locktmpwe 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_bytesmethod when available to save on some more copies.