Skip to content

perf: add reference_internal policy for const-ref returns#182

Merged
dellaert merged 2 commits into
borglab:masterfrom
jashshah999:perf/reference-return-policy
May 26, 2026
Merged

perf: add reference_internal policy for const-ref returns#182
dellaert merged 2 commits into
borglab:masterfrom
jashshah999:perf/reference-return-policy

Conversation

@jashshah999
Copy link
Copy Markdown
Contributor

Summary

  • Methods returning const T& now emit py::return_value_policy::reference_internal in the generated pybind11 bindings
  • The generated lambda uses -> const auto& trailing return type to preserve the reference semantics
  • This avoids unnecessary copies when Python accesses C++ members by const reference, keeping the returned reference tied to the parent object's lifetime

Motivation

Without this policy, pybind11 defaults to copy for returned references from lambdas (since the lambda return type is deduced as a value). This causes:

  1. Unnecessary deep copies of potentially large objects (matrices, vectors)
  2. Surprising semantics where modifying the "reference" in Python doesn't affect the C++ object

With reference_internal, pybind11 returns a reference and ties the Python reference's lifetime to the parent object, matching C++ semantics.

Changes

  • gtwrap/pybind_wrapper.py: Detect is_ref on the return type and emit -> const auto& + py::return_value_policy::reference_internal for instance methods
  • tests/expected/python/class_pybind.cpp: Updated expected output for return_vector2 and return_matrix2 const-ref overloads

Test plan

  • All existing tests pass (pytest tests/test_pybind_wrapper.py - 9/9 passing)
  • Verify with a downstream project (e.g. GTSAM) that const-ref accessors no longer copy

Methods returning const T& now generate lambdas with
`-> const auto&` return type and `py::return_value_policy::reference_internal`,
avoiding unnecessary copies and keeping the reference tied to the parent object lifetime.
Copy link
Copy Markdown
Member

@dellaert dellaert left a comment

Choose a reason for hiding this comment

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

@varunagrawal might be too busy - sorry. My comment is: please add a unit test to (a) show me how emitted code differs, and (b) verify that for posterity.

Adds test_const_ref_return_policy that explicitly checks:
- Methods returning const T& emit `-> const auto&` trailing return
- Methods returning const T& emit `py::return_value_policy::reference_internal`
- Methods returning by value (T, not T&) do NOT get the policy
@dellaert
Copy link
Copy Markdown
Member

Thanks for adding the test; it covers the intended const T& case well.

One issue with the implementation: the generator currently checks only return_type.type1.is_ref, while the behavior and comments are specifically about const T&. That means a future method returning a non-const mutable reference, e.g. SomeType& mutableThing(), would also be emitted as -> const auto& with py::return_value_policy::reference_internal. That would accidentally turn a mutable C++ reference into a const reference in the Python binding.

Could you either gate this path on both is_ref and is_const, or add a separate non-const-reference path/test if mutable reference returns are meant to be supported? For this PR as written, the conservative fix is probably to require type1.is_const before emitting -> const auto&.

@dellaert
Copy link
Copy Markdown
Member

Actually, I will add that in a follow up PR. Don;t pull wrap into gtsam yet.

@dellaert dellaert merged commit 41e4e62 into borglab:master May 26, 2026
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