[PR] python: avoid utf8 copy during fastbinary string encode [thrift]

["markjm (via GitHub)" <gi...@apache.org>]

markjm opened a new pull request, #3595:
URL: https://github.com/apache/thrift/pull/3595

   Hi - I figured I'd share a few perf optimizations we are using internally. We are still on an older thrift version (😢 ), so I did use some agent magic to port these to the head of this repo. My testing was primarily on my branch - in this case, for example, we only care about p3.12+, so i added some compat for when they added PyUnicode_AsUTF8AndSize in py3.3 (impressed the repo still suports py2!)
   
   This one is 1 of 3 PRs
   
   ---
   
   Use PyUnicode_AsUTF8AndSize when available, with a fallback for older Python 3 releases, so UTF-8 strings can be encoded without allocating an intermediate PyBytes object.
   
   Performance (50k iterations, warmed)
   
   | Workload | Baseline | This commit | Speedup |
   |----------|----------|-------------|---------|
   | encode simple (30B, 1 string) | 0.60 us | 0.55 us | 1.09x | | encode 10-string (182B) | 1.44 us | 1.01 us | 1.43x | | encode complex (395B) | 3.02 us | 2.56 us | 1.18x |
   
   The more string fields a struct has, the larger the gain. Decode is unchanged.
   


-- 
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] python: avoid utf8 copy during fastbinary string encode [thrift]

["markjm (via GitHub)" <gi...@apache.org>]

markjm commented on code in PR #3595:
URL: https://github.com/apache/thrift/pull/3595#discussion_r3407597543


##########
lib/py/src/ext/protocol.tcc:
##########
@@ -456,18 +456,32 @@ bool ProtocolBase<Impl>::encodeValue(PyObject* value, TType type, PyObject* type
 
   case T_STRING: {
     ScopedPyObject nval;
+    Py_ssize_t len;
 
     if (PyUnicode_Check(value)) {
+#if PY_VERSION_HEX >= 0x03030000
+      const char* str = PyUnicode_AsUTF8AndSize(value, &len);
+      if (!str) {
+        return false;
+      }
+      if (!detail::check_ssize_t_32(len)) {
+        return false;
+      }
+
+      impl()->writeI32(static_cast<int32_t>(len));
+      return writeBuffer(const_cast<char*>(str), static_cast<size_t>(len));

Review Comment:
   thanks bot



-- 
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-6069: python: avoid utf8 copy during fastbinary string encode [thrift]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #3595:
URL: https://github.com/apache/thrift/pull/3595#discussion_r3469348936


##########
lib/py/test/thrift_TSerializer.py:
##########
@@ -70,6 +70,17 @@ def test_TCompactProtocolAccelerated(self):
         factory = TCompactProtocolAcceleratedFactory()
         self.verify(self.compact_serialized, factory)
 
+    def test_TCompactProtocolAccelerated_unicode_matches_python(self):
+        message = Message("é", 42)

Review Comment:
   The new test uses a non-ASCII literal ("é") without an encoding cookie. This can make the test file fail to parse under Python 2 (default ASCII source encoding) and also makes it ambiguous whether `message.body` is intended to be `unicode` vs `bytes` on Py2. Prefer using an explicit unicode escape literal so the test is source-encoding-independent and always exercises the unicode path.



##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -280,6 +296,28 @@ def test_TBinaryProtocol_write_read(self):
             print("Assertion fail")
             raise e
 
+    def test_accelerated_utf8_roundtrip_on_application_exception(self):
+        if not self._has_fastbinary:
+            self.skipTest("C extension not available")
+
+        original = TApplicationException(
+            type=TApplicationException.PROTOCOL_ERROR,
+            message=("snowman-\u2603-rocket-\U0001F680-" * 32),

Review Comment:
   `message` is currently a plain string literal containing `\u`/`\U` escapes. On Python 2 this may fail to parse (or not produce a `unicode` object), and it also won’t reliably exercise the unicode encoding path. Make this an explicit unicode literal so the test is version-independent and consistently tests UTF-8 encoding of unicode.



-- 
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-6069: python: avoid utf8 copy during fastbinary string encode [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G merged PR #3595:
URL: https://github.com/apache/thrift/pull/3595


-- 
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-6069: python: avoid utf8 copy during fastbinary string encode [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G commented on PR #3595:
URL: https://github.com/apache/thrift/pull/3595#issuecomment-4698131382

   ### Code review
   
   Found 2 issues:
   
   1. The fast path calls `impl()->writeI32(len)` to write the string length, which is correct for `BinaryProtocol` but wrong for `CompactProtocol`. `CompactProtocol::writeString` encodes the length with `writeVarint(len)` (plain unsigned varint), while `writeI32` encodes with `writeVarint(toZigZag(val))` (zigzag). The fast path will produce a malformed Compact frame for any unicode string field, and a decoder will read the wrong number of bytes.
   
   https://github.com/apache/thrift/blob/0691570336b46b4eb5f83bc24c57012de9c68f0e/lib/py/src/ext/protocol.tcc#L469-L473
   
   2. AI tool use is acknowledged in the PR body but neither commit contains the required `Co-Authored-By:` or `Generated-by:` label ([AGENTS.md §4](https://github.com/apache/thrift/blob/35c1a53dd6de205a4a7c23fc356f1fba490116c4/AGENTS.md#L57-L71) says "**Always** label AI-assisted commits and PRs … Apply this label even when AI only generated a portion of the change").
   
   https://github.com/apache/thrift/blob/35c1a53dd6de205a4a7c23fc356f1fba490116c4/AGENTS.md#L57-L71
   
   🤖 Generated with [Claude Code](https://claude.ai/code)
   
   <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub>


-- 
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-6069: python: avoid utf8 copy during fastbinary string encode [thrift]

["Jens-G (via GitHub)" <gi...@apache.org>]

Jens-G commented on PR #3595:
URL: https://github.com/apache/thrift/pull/3595#issuecomment-4745932561

   Hi, any plans to continue here?


-- 
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] python: avoid utf8 copy during fastbinary string encode [thrift]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #3595:
URL: https://github.com/apache/thrift/pull/3595#discussion_r3407591011


##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -22,7 +22,9 @@
 import uuid
 
 import _import_local_thrift  # noqa
+from thrift.Thrift import TApplicationException
 from thrift.protocol.TBinaryProtocol import TBinaryProtocol
+from thrift.protocol.TBinaryProtocol import TBinaryProtocolAcceleratedFactory

Review Comment:
   Importing `TBinaryProtocolAcceleratedFactory` at module import time can make the entire test module fail to import in environments where that symbol isn’t present, even though the test itself is guarded by `fastbinary` availability. Consider importing `TBinaryProtocolAcceleratedFactory` inside `setUp()` or the specific test with a `try/except ImportError` and skipping the test (or the module) when unavailable.



##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -280,6 +299,26 @@ def test_TBinaryProtocol_write_read(self):
             print("Assertion fail")
             raise e
 
+    def test_accelerated_utf8_roundtrip_on_application_exception(self):
+        if not self._has_fastbinary:
+            self.skipTest("C extension not available")
+
+        original = TApplicationException(
+            type=TApplicationException.PROTOCOL_ERROR,
+            message=("snowman-\u2603-rocket-\U0001F680-" * 32),
+        )
+
+        otrans = TTransport.TMemoryBuffer()
+        oproto = TBinaryProtocolAcceleratedFactory(fallback=False).getProtocol(otrans)
+        oproto.trans.write(oproto._fast_encode(original, APPLICATION_EXCEPTION_TYPEARGS))
+
+        itrans = TTransport.TMemoryBuffer(otrans.getvalue())
+        iproto = TBinaryProtocolAcceleratedFactory(fallback=False).getProtocol(itrans)
+        decoded = iproto._fast_decode(None, iproto, APPLICATION_EXCEPTION_TYPEARGS)

Review Comment:
   This test calls private APIs (`_fast_encode` / `_fast_decode`) directly. That’s brittle across Thrift versions and can fail even when `fastbinary` is present but the protocol instance doesn’t expose these methods the same way. Consider guarding with `hasattr(oproto, \"_fast_encode\")` / `hasattr(iproto, \"_fast_decode\")` and skipping if absent, or using a public accelerated encode/decode entrypoint if the library provides one.



##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -280,6 +299,26 @@ def test_TBinaryProtocol_write_read(self):
             print("Assertion fail")
             raise e
 
+    def test_accelerated_utf8_roundtrip_on_application_exception(self):
+        if not self._has_fastbinary:
+            self.skipTest("C extension not available")
+
+        original = TApplicationException(
+            type=TApplicationException.PROTOCOL_ERROR,
+            message=("snowman-\u2603-rocket-\U0001F680-" * 32),
+        )
+
+        otrans = TTransport.TMemoryBuffer()
+        oproto = TBinaryProtocolAcceleratedFactory(fallback=False).getProtocol(otrans)
+        oproto.trans.write(oproto._fast_encode(original, APPLICATION_EXCEPTION_TYPEARGS))

Review Comment:
   This test calls private APIs (`_fast_encode` / `_fast_decode`) directly. That’s brittle across Thrift versions and can fail even when `fastbinary` is present but the protocol instance doesn’t expose these methods the same way. Consider guarding with `hasattr(oproto, \"_fast_encode\")` / `hasattr(iproto, \"_fast_decode\")` and skipping if absent, or using a public accelerated encode/decode entrypoint if the library provides one.



##########
lib/py/src/ext/protocol.tcc:
##########
@@ -456,18 +456,32 @@ bool ProtocolBase<Impl>::encodeValue(PyObject* value, TType type, PyObject* type
 
   case T_STRING: {
     ScopedPyObject nval;
+    Py_ssize_t len;
 
     if (PyUnicode_Check(value)) {
+#if PY_VERSION_HEX >= 0x03030000
+      const char* str = PyUnicode_AsUTF8AndSize(value, &len);
+      if (!str) {
+        return false;
+      }
+      if (!detail::check_ssize_t_32(len)) {
+        return false;
+      }
+
+      impl()->writeI32(static_cast<int32_t>(len));
+      return writeBuffer(const_cast<char*>(str), static_cast<size_t>(len));

Review Comment:
   Using `const_cast<char*>` here is avoidable and potentially unsafe if `writeBuffer` ever writes into the buffer (even accidentally). Prefer changing `writeBuffer` to accept a `const char*` (or adding an overload) and pass `str` through without casting. If changing the signature isn’t feasible, a safe alternative is to copy into a mutable buffer before calling `writeBuffer` (with the tradeoff of allocating).



##########
lib/py/test/thrift_TBinaryProtocol.py:
##########
@@ -167,6 +169,16 @@ def testField(type, data):
     protocol.readStructEnd()
 
 
+APPLICATION_EXCEPTION_TYPEARGS = [
+    TApplicationException,
+    (
+        None,
+        (1, 11, "message", "UTF8", None),
+        (2, 8, "type", None, None),
+    ),
+]

Review Comment:
   `*_TYPEARGS` in Thrift’s accelerated paths are typically tuples (and may be used as cache keys or assumed immutable by downstream helpers). Using a list here is more error-prone and can break if internals assume tuple semantics. Consider making this a tuple (e.g., `(..., (...))`) to match common Thrift fastbinary conventions and avoid accidental mutation between tests.



-- 
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