Use String#append_bytes if available
#116
Conversation
|
Nice results. Detecting if the method is defined and using it seems fine? Having an option hash leaves room for people to forget to enable it? |
|
The option hash is so that you can tell protoboeuf: yes I'm running you with a Ruby that has |
|
This seems good to me (as a POC). It depends on this ticket being completed though, right? (I think I'm right I just want to link to the ticket so it's easier to track) |
Yes. Implementation here: ruby/ruby#11293 The general idea have been approved, but we'll probably need to wait at least for September 5th meeting to clear up the last remaining question and get a full approval. So until then, little point merging. Might be worth commenting on the upstream ticket that we validated the effectiveness with that benchmark. |
👍 I'll leave a comment on the ticket |
casperisfine
force-pushed
the
append-bytes
branch
from
0bbd7d8 to
7f75616
Compare
casperisfine
force-pushed
the
append-bytes
branch
2 times, most recently
from
97504f0 to
c0ff04c
Compare
|
So I've rebased both this branch and my Ruby PR. I'm no longer seeing a really significant gain. It's consistently faster, but no longer by enough to be considered statistically significant by I'm not quite certain why, maybe it's the various optimizations I did two weeks ago? But that's weird because if anything that should make this part proportionally more important. If I profile with Vernier (with YJIT disabled), 
So I'm a bit puzzled here. |
|
Nevermind, I figured it out 30 seconds after writing it down. Looking at the initial benchmark variance was quite low I suspect it's GC. If I clear the encoded string to appease the GC, it's much better: x.report("pboeuf-edge#{version}") { edge_decoded_msgs_proto.each { |msg| ProtoBoeufEdge::ParkingLot.encode(msg).clear } }and |
|
With the comparison against |
This is just a proof of concept / demo, there are some unknown
about how codegen is supposed to know if it's OK to use newly introduced
methods.
I also hacked the benchmark to load two versions of protoboeuf so
I can compare them together, that's proable not how we want it
but it gives a much clearer picture of the speedup.
```
/opt/rubies/head/bin/ruby --yjit -I lib:bench/lib bench/benchmark.rb
total encoded size: 5038040 bytes
=== encode ===
ruby 3.4.0dev (2024-08-26T08:40:45Z string-append-bytes 28a1b94c15) +YJIT [arm64-darwin23]
Warming up --------------------------------------
upstream/jit 13.000 i/100ms
protoboeuf/jit 5.000 i/100ms
pboeuf-edge/jit 5.000 i/100ms
Calculating -------------------------------------
upstream/jit 126.321 (± 4.7%) i/s - 637.000 in 5.053302s
protoboeuf/jit 51.886 (± 3.9%) i/s - 260.000 in 5.017310s
pboeuf-edge/jit 58.609 (± 3.4%) i/s - 295.000 in 5.041832s
Comparison:
upstream/jit: 126.3 i/s
pboeuf-edge/jit: 58.6 i/s - 2.16x slower
protoboeuf/jit: 51.9 i/s - 2.43x slower
```
casperisfine
force-pushed
the
append-bytes
branch
from
c0ff04c to
e2aeb72
Compare
Final version of: #116 Ref: https://bugs.ruby-lang.org/issues/20594 This isn't yet as fast as the prototype because the final method was merged as `append_as_bytes` with variadic and Integer support so I haven't implemented a YJIT acceleration yet. But since we can now consider the interface stable, we can merge.
|
Closing in favor of #158 |
Final version of: #116 Ref: https://bugs.ruby-lang.org/issues/20594 This isn't yet as fast as the prototype because the final method was merged as `append_as_bytes` with variadic and Integer support so I haven't implemented a YJIT acceleration yet. But since we can now consider the interface stable, we can merge.
This is just a proof of concept / demo, there are some unknown about how codegen is supposed to know if it's OK to use newly introduced methods. So I added a simple option hash, but perhaps what would make sense in the future is to pass a Ruby version, to act as a minimum supported version?
I also hacked the benchmark to load two versions of protoboeuf so I can compare them together, that's proable not how we want it but it gives a much clearer picture of the speedup.