Skip to content

perf(hiroz-py): remove per-message Python work from the receive path - #251

Open
YuanYuYuan wants to merge 5 commits into
mainfrom
perf/py-receive-path
Open

perf(hiroz-py): remove per-message Python work from the receive path#251
YuanYuYuan wants to merge 5 commits into
mainfrom
perf/py-receive-path

Conversation

@YuanYuYuan

@YuanYuYuan YuanYuYuan commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two independent optimisations of the hiroz-py receive path. Both remove per-message Python work that codegen already determines.

Note

No failing baseline, and none claimed. The justification is the measured saving below. The guard tests stop the optimisation regressing. They do not demonstrate a defect.

tag before after file
C1 deserialize_message walked the Python REGISTRY per message: six C-API lookups plus one Python-level call deserialize_direct dispatches codegen-known types through a Rust match, mirroring serialize_to_zbuf generate_deserialize_direct in python_msgspec_generator.rs
C2 into_py_message re-resolved its msgspec class per call, once per nested message, plus one throwaway PyUnicode per kwargs key class cached in a per-impl GILOnceCell; every kwargs key wrapped in intern! impl_into_py_message and generate_field_construction in hiroz-derive/src/lib.rs

A sensor_msgs/JointState paid the C2 cost three times: JointState, Header, Time.

Design points a reviewer may question

  • Each match arm delegates to the per-type deserialize_<name> that generate_deserialize_function already emits. Inlining would give three copies of the same header handling, endianness choice and error mapping.
  • The wildcard arm falls back to the registry rather than erroring. REGISTRY is a live, mutable Python dict, and create_subscriber accepts any class carrying __msgtype__. Erroring would silently withdraw runtime registration: hiroz would still create the subscriber, then every callback would raise Unknown message type. This is a deliberate asymmetry with serialize_to_zbuf, whose wildcard does error.
  • deserialize_direct validates the 4-byte encapsulation header before dispatch, so the fallback reports the same error as the direct path.
  • The exported deserialize_message pyfunction stays registry-backed. It is the escape hatch for types resolved at runtime.

Evidence

C1 — JointState ping-pong, inter-process through a router, half-trip p50, 5 interleaved reps x 10 s per arm, per-percentile median across reps:

payload baseline patched saving
empty 56.0 µs 52.3 µs 3.7 µs
joint12 81.0 µs 76.7 µs 4.3 µs
joint100 134.9 µs 130.1 µs 4.9 µs

The patched build is faster in 15 of 15 paired reps. The per-rep ranges do not overlap at any payload: at joint12, baseline 80.9–82.4 µs against patched 76.0–77.5 µs. The saving stays near-constant while total latency grows from 56 to 135 µs. That is the signature of fixed dispatch overhead, not of serialization work. The mild residual slope (3.7 → 4.9 µs) has a known cause: the old path also built a PyBytes copy that scales with size.

C2 — the generated per-type decoder called directly from Python, 8 000 iterations, 5 interleaved reps per arm, median-of-percentile across reps, µs:

payload before after delta per-rep ranges disjoint
empty 2.51 0.62 −1.90 yes
joint1 9.79 3.27 −6.52 yes
joint12 14.17 7.20 −6.97 yes
joint100 46.23 37.99 −8.25 yes
bytes64 6.05 2.03 −4.03 yes
bytes4k 6.28 2.22 −4.06 yes
bytes64k 9.25 4.91 −4.34 yes

The send-side encoder is the control. It does not move (joint12 4.95 → 4.82, joint100 19.59 → 19.48), so the effect is not drift between the two builds. The saving tracks nesting depth, not payload size: 1.9 µs for Empty (one class), ~4.0 µs for ByteMultiArray (two), ~7 µs for JointState (three). The JointState saving is the same ~7 µs at 1 field or at 100. A payload-copy removal would show the opposite signature. The two mechanisms are distinguishable from the measurements alone.

perf corroborates the named mechanism for C2. Same cell, cycles, leaf attribution:

symbol before after
PyImport_ImportModuleLevelObject 0.26% absent
PyImport_Import 0.20% absent
import_get_module 0.10% absent
import_ensure_initialized 0.10% absent
_Py_module_getattro 0.05% absent
_PyObject_GenericGetAttrWithDict 0.66% 0.05%
PyDict_GetItemRef 0.50% 0.02%
PyUnicode_New 0.32% 0.12%

CI on head commit 5e2d6d893ba1a514d5d1555a9a91c2aa285e4845: 28 check runs completed, all green; license/cla also green. No pending and no failed checks.

Guard coverage

crates/hiroz-py/tests/test_deserialize_dispatch.py adds three tests. They use std_msgs/String, which is codegen-known, so they exercise the direct path specifically.

test detects
test_callback_subscriber_decodes_without_registry empties REGISTRY, publishes through a real subscriber; fails on an unpatched build — the callback never fires
test_recv_subscriber_decodes_without_registry the same, on the recv() path; fails on an unpatched build — recv raises out of the registry walk
test_exported_deserialize_message_still_uses_registry pins the deliberate asymmetry; passes on both builds by design, and fails only if the escape hatch is rewired

The fixture restores REGISTRY afterwards. It asserts the registry was non-empty first, so the detector cannot be vacuous.

C2 has no dedicated test. The existing hiroz-py suite covers it, including test_complex_messages.py, which round-trips the nested and sequence-of-nested shapes it touches.

Breaking changes

None. The table records the behaviour deltas a reviewer may still want to see.

behaviour before after
Codegen-known types on the subscriber path resolved through REGISTRY at runtime resolved by a Rust match; identical result
Types registered at runtime resolved through REGISTRY unchanged — the wildcard falls back to REGISTRY
deserialize_message pyfunction registry-backed unchanged
CDR buffer shorter than 4 bytes on the registry path whatever the Python decoder raised ValueError: CDR data too short: missing encapsulation header, the same message the direct path already used

Note

GILOnceCell carries the usual caveat that the interpreter does not drop the cached object on finalization. This is the same trade-off intern! already makes, and the extension targets a single long-lived interpreter.

Coverage this does not have

None of these block the change. They bound what green CI proves.

tag gap
G1 No dedicated regression test for C2. Its benefit is measured, not asserted. A future revert would show up as a latency change, not a test failure.
G2 The measurements are single-host. There are no cross-platform or cross-Python-version numbers.
G3 The registry fallback arm is exercised only through deserialize_message, not through a subscriber on a runtime-registered type.
G4 ./scripts/check-local.sh was not re-run after the final three commits. The CI run on the head commit covers the same gates.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Optimizes Python message reception by replacing repeated runtime lookups with generated Rust dispatch and cached Python objects.

Changes:

  • Adds direct Rust deserialization dispatch.
  • Caches msgspec classes and interns field names.
  • Adds subscriber dispatch regression tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
crates/hiroz-codegen/src/python_msgspec_generator.rs Generates direct receive-side deserialization.
crates/hiroz-derive/src/lib.rs Caches classes and interns keyword keys.
crates/hiroz-py/tests/test_deserialize_dispatch.py Verifies generated subscriber types bypass the registry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/hiroz-codegen/src/python_msgspec_generator.rs Outdated
Comment thread crates/hiroz-codegen/src/python_msgspec_generator.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

crates/hiroz-codegen/src/python_msgspec_generator.rs:739

  • No test exercises this wildcard through deserialize_from_cdr: test_exported_deserialize_message_still_uses_registry invokes deserialize_message directly. Consequently, replacing this fallback with an error would leave all new tests green while breaking runtime-registered subscriber types. Add an integration test with a custom __msgtype__/REGISTRY entry and publish_raw() to verify this receive path.
                // Not known at codegen time: defer to the runtime registry.
                // `deserialize_message` takes the *whole* buffer, header
                // included — it hands it to the type's own Python
                // `deserialize`, which strips the header itself.
                _ => unsafe { deserialize_message(py, type_name, bytes) },

crates/hiroz-codegen/src/python_msgspec_generator.rs:690

  • The registry-bypass guards only use std_msgs/String, so neither generated service arm is tested. The existing service round-trip runs with a populated registry, meaning a missing service arm would silently fall back and still pass while losing this optimization. Add a request/response test with the registry deserializers removed or poisoned, covering both take_request() and the client response.
        let req_full_name = format!("{}/srv/{}_Request", srv.parsed.package, srv.parsed.name);
        let req_name_ident = format_ident!("{}Request", srv.parsed.name);
        match_arms.push(quote! {
            #req_full_name => {

crates/hiroz-derive/src/lib.rs:267

  • GILOnceCell initializes this process-global static only once, not once per interpreter. Since the implementation intentionally assumes one long-lived interpreter, state that explicitly rather than implying subinterpreter-safe per-interpreter caching.
                // `GILOnceCell` resolves the class once per interpreter and
                // hands back a borrowed reference thereafter.

crates/hiroz-py/tests/test_deserialize_dispatch.py:7

  • The direct path still constructs Python classes, dictionaries, keys, and field values under the GIL; it only avoids the Python registry dispatch. Saying it does not touch Python state at all overstates the optimization and contradicts the class-caching change.
deliberately bypassing ``hiroz_py.hiroz_msgs.REGISTRY``. ``deserialize_from_cdr``
mirrors it for every **codegen-known** type: a subscriber decodes an incoming
sample of such a type without touching Python state at all.

@YuanYuYuan
YuanYuYuan force-pushed the perf/py-receive-path branch from 6df6a8a to 5e2d6d8 Compare July 28, 2026 14:23
serialize_to_zbuf already dispatches via a Rust match, explicitly to
bypass the Python registry. deserialize_message did the opposite, per
message, under the callback's re-acquired GIL:

  import sys -> getattr modules -> get_item("hiroz_py.hiroz_msgs")
             -> getattr REGISTRY -> get_item(type_name)
             -> get_item("deserialize") -> call1((bytes,))

Six C-API round trips, two transient PyStrings and a Python-level call,
to reach a function that is fixed at codegen time. This adds
generate_deserialize_direct() as the receive-side mirror of
generate_serialize_to_zbuf().

The exported deserialize_message pyfunction deliberately stays on the
registry - it is the escape hatch for types resolved at runtime. Only the
codegen-time path (deserialize_from_cdr, used by pubsub.rs, node.rs and
service.rs) moves to the direct match.

Measured on a JointState ping-pong, inter-proc-local, half-trip p50,
5 interleaved reps x 10s per arm, per-percentile median across reps:

  payload    baseline   patched   saving
  empty        56.0us    52.3us    3.7us
  joint12      81.0us    76.7us    4.3us
  joint100    134.9us   130.1us    4.9us

The patched build is faster in 15 of 15 paired reps, and the per-rep
ranges of the two arms do not overlap at any payload (e.g. joint12
baseline 80.9-82.4us vs patched 76.0-77.5us), so the effect is separable
from run-to-run noise rather than an artifact of which sample landed in
the middle.

The saving is near-constant while total latency grows 56 -> 135us, which
is the signature of fixed dispatch overhead rather than serialization
work. The mild residual slope (3.7 -> 4.9us) is expected: the old path
also built a PyBytes copy of the payload, and that part does scale with
message size.

tests/test_deserialize_dispatch.py guards the change by emptying
hiroz_msgs.REGISTRY and pushing a message through a real subscriber, on
both the callback and recv paths. Verified to detect in both directions:
the tests fail on an unpatched build ("Unknown message type" out of the
registry walk, no delivery) and pass on a patched one.
`IntoPyMessage::into_py_message` is the whole receive-side Rust -> Python
conversion, and it re-resolved its target class on every call:

    PyModule::import_bound(py, "hiroz_msgs_py.types.<pkg>")
        -> getattr("<Name>")

`import_bound` builds a transient `PyUnicode` from the module path and enters
the import machinery; `getattr` builds another `PyUnicode` and walks the
module's dict. Both run under the subscriber callback's re-acquired GIL, and
both run once per *nested* message: a `sensor_msgs/JointState` pays it three
times (JointState, Header, Time), a `std_msgs/ByteMultiArray` twice. The class
is fixed at codegen time, so none of that work can change its answer.

`kwargs.set_item(<&str>, ...)` had the same shape one level down — pyo3 builds
a fresh `PyUnicode` for a `&str` key on every call, so a 5-field message
allocated 5 throwaway strings per construction.

Fix: a per-impl `GILOnceCell` for the class, and `intern!` for the kwargs keys.

Isolated measurement of the generated per-type decoder (`REGISTRY[t]["deserialize"]`
called directly from Python, 8 000 iterations, 5 interleaved reps per arm,
median-of-percentile across reps, us):

    payload    p50 before   p50 after   delta   per-rep ranges disjoint
    empty            2.51        0.62   -1.90   yes
    joint1           9.79        3.27   -6.52   yes
    joint12         14.17        7.20   -6.97   yes
    joint100        46.23       37.99   -8.25   yes
    bytes64          6.05        2.03   -4.03   yes
    bytes4k          6.28        2.22   -4.06   yes
    bytes64k         9.25        4.91   -4.34   yes

The send-side encoder is the control and does not move (joint12 4.95 -> 4.82,
joint100 19.59 -> 19.48), so the effect is not drift between the two builds.

The saving tracks *nesting depth*, not payload size: 1.9 us for Empty (one
class), ~4.0 us for ByteMultiArray (two), ~7 us for JointState (three), and it
is the same ~7 us whether the JointState carries 1 field or 100. That is the
signature of fixed per-construction overhead, which is what was removed.

perf corroborates the named mechanism. Same cell, cycles, leaf attribution:

    symbol                              before   after
    PyImport_ImportModuleLevelObject     0.26%   absent
    PyImport_Import                      0.20%   absent
    import_get_module                    0.10%   absent
    import_ensure_initialized            0.10%   absent
    _Py_module_getattro                  0.05%   absent
    _PyObject_GenericGetAttrWithDict     0.66%    0.05%
    PyDict_GetItemRef                    0.50%    0.02%
    PyUnicode_New                        0.32%    0.12%

`GILOnceCell` carries the usual caveat that the cached object is not dropped on
interpreter finalization — the same trade-off `intern!` already makes, and the
extension is built for a single long-lived interpreter.

Covered by the existing hiroz-py suite (52 passed), including
`test_complex_messages.py`, which round-trips the nested and sequence-of-nested
shapes this touches.
The direct-dispatch receive path replaced a runtime REGISTRY lookup with a
match generated at codegen time, and made its wildcard an error. That
silently withdrew a supported capability.

`deserialize_message` resolves `sys.modules["hiroz_py.hiroz_msgs"].REGISTRY`
at call time, and REGISTRY is a live, mutable Python dict. `create_subscriber`
accepts any class carrying `__msgtype__` and documents itself as working with
"any registered message type". So before this branch, a type registered after
module init decoded normally. With an erroring wildcard the subscriber still
builds and then raises `Unknown message type` on every callback -- a failure
that surfaces only at the first message.

Fall back to `deserialize_message` for anything the codegen did not see.
Known types keep the fast Rust path; everything else behaves exactly as it
did. The fallback passes the whole buffer, header included, because the
type's own Python `deserialize` strips it.

No regression test accompanies this. Exercising the fallback end to end needs
a publisher of a non-codegen type, and the send path (`serialize_to_zbuf`)
has bypassed the registry since before this branch -- so a Python-side
publisher cannot produce one. The gap is stated rather than papered over.
…at them

Each arm of `deserialize_direct` inlined the full decode -- header skip,
endianness, error mapping, `into_py_message` -- duplicating what
`generate_deserialize_function` already emits per type, and the request and
response arms duplicated it again. Three copies that a future fix to any of
those steps could leave inconsistent.

Arms now call the existing `deserialize_<name>` directly. The fast path is
unchanged: still a plain Rust call with no Python registry and no Python-level
dispatch. Those functions take the whole buffer because they strip the
encapsulation header themselves, so the local `payload` slice is gone; the
length check stays, since the registry fallback does not make one.
…tfmt

The GitHub formatting gate runs `nix build .#checks...pre-commit-check`,
which pins a stricter rustfmt than the local `cargo fmt --all --check`.
This line exceeded its width; no behaviour change.
@YuanYuYuan
YuanYuYuan force-pushed the perf/py-receive-path branch from 5e2d6d8 to 848057d Compare August 14, 2026 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants