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?
ext/openssl/ossl_ssl.c
Outdated
| @@ -2065,14 +2065,16 @@ ossl_ssl_write_internal(VALUE self, VALUE str, VALUE opts) | |||
| if (!ssl_started(ssl)) | |||
| rb_raise(eSSLError, "SSL session is not started yet"); | |||
|
|
|||
| tmp = rb_str_new_frozen(StringValue(str)); | |||
| tmp = rb_str_locktmp(StringValue(str)); | |||
There was a problem hiding this comment.
I think we want to skip rb_str_locktmp() if the String is already frozen. It currently works, but locking a frozen String seems strange. I think we're safe to assume RSTRING_PTR(str) won't change.
There was a problem hiding this comment.
We could indeed, but that makes the codepath more complicated for not much gain? Except maybe avoid mutating a frozen string whihc could have some small copy on write consequences. Perhaps it's more of a job for rb_str_locktmp to skip the lock if the string is already frozen?
There was a problem hiding this comment.
I think rb_str_locktmp() is not well defined for frozen strings (e.g. oracle/truffleruby#3752) so it seems best to avoid it.
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
[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
8db1bcd to
4e8df0f
Compare
[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.