You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "kpumuk (via GitHub)" <gi...@apache.org> on 2026/08/23 19:55:38 UTC
[PR] THRIFT-6153: Batch native Ruby Compact varint writes [thrift]
kpumuk opened a new pull request, #3741:
URL: https://github.com/apache/thrift/pull/3741
<!-- Explain the changes in the pull request below: -->
The native Ruby Compact Protocol previously called the transport's write method once per encoded byte for multibyte varints. This created up to five one-byte Ruby strings for an i32 and ten for an i64, along with the corresponding dynamic method dispatches.
This change encodes each varint into a bounded C stack buffer and passes it through the existing transport write boundary once. It preserves the Compact wire format and generic transport behavior without specializing the optimization for `MemoryBufferTransport`.
## Benchmarks
Measured on Ruby 4.0.6, aarch64-linux, using:
```text
ruby test/rb/benchmarks/protocol_benchmark.rb --json --scenarios rb-cmp-write-small,rb-cmp-read-small --small-runs 500000
```
The pure-Ruby control used the same command with `THRIFT_BENCHMARK_SKIP_NATIVE=1` and `--small-runs 150000`. Each timing is the median of three measured trials after one discarded warm-up.
| Scenario | master median (range) | proposed median (range) | Change |
| --- | ---: | ---: | ---: |
| Native Compact write | 2.666s (2.657–2.724s) | 2.523s (2.512–2.585s) | 5.3% faster |
| Native Compact read control | 2.639s (2.612–2.683s) | 2.673s (2.625–2.778s) | 1.3% slower |
| Pure-Ruby Compact write control | 2.260s (2.233–2.288s) | 2.260s (2.248–2.355s) | unchanged |
Allocations were measured around the same `rb-cmp-write-small` scenario with `GC.stat(:total_allocated_objects)`, using 500,000 writes, one discarded warm-up, and three measured trials.
| Allocation metric | master median (range) | proposed median (range) | Change |
| --- | ---: | ---: | ---: |
| Allocated objects per 500,000 writes | 16,500,007 (16,500,007–16,500,008) | 12,000,007 (12,000,007–12,000,008) | 27.3% fewer |
| Allocated objects per small struct | approximately 33 | approximately 24 | 9 fewer |
The benchmark serializes the small `OneOfEach` fixture through `MemoryBufferTransport` and measures this change as a standalone commit. The allocation count includes protocol and transport setup but measures objects rather than allocated bytes or peak memory. These results do not cover large payloads or imply the earlier aggregate 2× result.
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [x] Did you create an [Apache Jira](https://issues.apache.org/jira/browse/THRIFT-6153) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)
- [x] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [x] Did you squash your changes to a single commit? (not required, but preferred)
- [x] Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
- [ ] If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6153: Batch native Ruby Compact varint writes [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk merged PR #3741:
URL: https://github.com/apache/thrift/pull/3741
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6153: Batch native Ruby Compact varint writes [thrift]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3741:
URL: https://github.com/apache/thrift/pull/3741#discussion_r3839453969
##########
lib/rb/ext/compact_protocol.c:
##########
@@ -174,27 +174,29 @@ static int32_t message_seqid_from_varint32(uint32_t seqid) {
}
static void write_varint32(VALUE transport, uint32_t n) {
- while (true) {
- if ((n & ~0x7FU) == 0U) {
- write_byte_direct(transport, n & 0x7FU);
- break;
- } else {
- write_byte_direct(transport, (n & 0x7FU) | 0x80U);
- n = n >> 7;
- }
+ unsigned char bytes[5];
+ long length = 0;
+
+ while ((n & ~0x7FU) != 0U) {
+ bytes[length++] = (n & 0x7FU) | 0x80U;
+ n >>= 7;
}
+ bytes[length++] = n;
+
+ WRITE(transport, (const char*)bytes, length);
Review Comment:
This changes an observable transport boundary, but the existing compact-protocol specs only verify the final bytes and do not detect a regression back to one `write` call per byte. Please add a native-path transport spy covering multi-byte i32 and i64 values and asserting that each varint is delivered in one write containing the complete encoding; this is the behavior the optimization is intended to guarantee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6153: Batch native Ruby Compact varint writes [thrift]
Posted by "kpumuk (via GitHub)" <gi...@apache.org>.
kpumuk commented on code in PR #3741:
URL: https://github.com/apache/thrift/pull/3741#discussion_r3839466948
##########
lib/rb/ext/compact_protocol.c:
##########
@@ -174,27 +174,29 @@ static int32_t message_seqid_from_varint32(uint32_t seqid) {
}
static void write_varint32(VALUE transport, uint32_t n) {
- while (true) {
- if ((n & ~0x7FU) == 0U) {
- write_byte_direct(transport, n & 0x7FU);
- break;
- } else {
- write_byte_direct(transport, (n & 0x7FU) | 0x80U);
- n = n >> 7;
- }
+ unsigned char bytes[5];
+ long length = 0;
+
+ while ((n & ~0x7FU) != 0U) {
+ bytes[length++] = (n & 0x7FU) | 0x80U;
+ n >>= 7;
}
+ bytes[length++] = n;
+
+ WRITE(transport, (const char*)bytes, length);
Review Comment:
Added a native-path recording transport regression in 19af32b91. It covers maximum-length i32 and i64 varints and asserts that each is delivered as a single write containing the complete canonical encoding.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org