Skip to content

Python: always pass the doc comment as a possibly-null pointer - #47

Merged
adalisk-emikhaylov merged 2 commits into
masterfrom
py-small-wins
Aug 25, 2026
Merged

Python: always pass the doc comment as a possibly-null pointer#47
adalisk-emikhaylov merged 2 commits into
masterfrom
py-small-wins

Conversation

@Fedr

@Fedr Fedr commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

A deliberately minimal alternative to #44: four functional lines, no structural change, and it still measurably shrinks the generated modules.

The idea

cpp_function is instantiated per set of pybind11 extras, so def(name, f, arg("x"), "a comment") and def(name, f, arg("x")) produce two separate instantiations even when the signature is identical. Since roughly half of MeshLib's bound entities carry a doc comment and half do not, that doubles a large number of shapes for no reason.

pybind11 treats a null const char * attribute exactly like an absent one - process_attribute<const char *>::init just assigns it to function_record::doc, and doc defaults to null anyway - so the comment can be passed unconditionally, as a null pointer when there is none. Entities with and without a comment then land on the same instantiation.

DETAIL_MB_PB11_COMMENT_PTR already existed for exactly this shape, so this is mostly a matter of using it at four more call sites: methods, free functions, fields and constructors. The (const char *) cast is needed because the macro's empty case expands to a bare nullptr, which would otherwise deduce as std::nullptr_t and fail to find a process_attribute specialization.

Results

Synthetic module (40 classes x 20 methods + 240 fields + 200 free functions, half of them commented and half not, to mirror the real mix), Clang 22, -Oz, no LTO or ICF, same generated .cpp:

.text delta
master 5d6f332e 4,270,502 -
+ methods and free functions 4,176,038 -2.21%
+ fields and constructors 4,172,838 -2.29%

MeshLib mrmeshpy.so, same MeshLib commit (e435a34c9) with the default MODE=release (so -Oz -flto=thin and -Wl,--icf=all), varying only the mrbind gitlink, baseline = master 5d6f332e:

compressed (in wheel) unpacked .text meshlib-core wheel
x86_64 -0.78% -0.34% -1.30% -0.22%
aarch64 -1.30% -1.30% -1.77% -0.35%

Absolute numbers, x86_64: compressed 15,573,029 -> 15,451,650, .text 29,345,354 -> 28,964,952. aarch64: compressed 14,056,202 -> 13,874,118, .text 17,687,800 -> 17,374,664.

Smaller than the synthetic benchmark predicted (-2.29% of .text there), which is expected - the real commented/uncommented mix is not 50/50 and many shapes are unique regardless. Still a measurable reduction for four lines, and unlike #44 it is a pure code reduction: no data or relocations are added, so the unpacked size drops too rather than staying flat.

.text is quoted because it is deterministic to the byte; on Windows stripped + xz is not reproducible across rebuilds of identical source, so it is not a usable metric at this scale.

Behavior is unchanged

A dump of pydoc.render_doc(), every attribute's repr and __doc__, every property's fget/fset/fdel with their __doc__ and __name__, plus ~50 live calls and field round-trips, is byte-identical to master's on two inputs - a feature test with a deliberate mix of commented and uncommented methods, free functions, fields, static fields and constructors, and a second input covering the standard containers and smart pointers. Also passes -fsyntax-only against the MeshInspector pybind11 fork with MeshLib's limited-API defines.

Relationship to the other PRs

Independent of, and much smaller than, #44. #44 contains this same change as one of its parts, so if #44 lands this one is redundant; if #44 is judged too invasive, this captures a small slice of the win on its own. It is also orthogonal to 5d6f332e - that one shrank the code inside TryAddFunc, this one reduces how many times cpp_function is instantiated - and it is measured on top of it.

One thing I tried and rejected

Sharing the _offsetof_* getter lambda instead of emitting a captureless one per field looked like an obvious win and is not one. A single shared capturing lambda costs +0.29% of .text and a per-field capturing one +1.1%, because master's +[] decays to a plain function pointer and hits pybind11's lean initialize overload, while any closure takes the generic one. The per-field lambda bodies it would replace are only a few instructions each. Master is already doing the right thing there. (I had listed this as a win in #44's description; that is now corrected.)

`cpp_function` is instantiated per set of pybind11 extras, so a function with
a doc comment and an otherwise identical one without it got two separate
instantiations. pybind11 treats a null `const char *` attribute exactly like
an absent one (`process_attribute<const char *>::init` just assigns it to
`function_record::doc`), so passing the comment unconditionally - as null when
there is none - collapses those two shapes into one.

Applies to methods, free functions, fields and constructors. Worth -2.3% of
`.text` on a synthetic module with a 50/50 mix of commented and uncommented
entities, for no change in behavior: `pydoc` output, every `__doc__`, and
every property's `fget`/`fset` docstrings are byte-identical.
@Fedr

Fedr commented Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

For context on how this compares with #44, all measured on the same MeshLib commit (e435a34c9), MODE=release, baseline master 5d6f332e:

x86_64

compressed unpacked .text
this PR (+11/-8 lines) -0.78% -0.34% -1.30%
#44 (+695/-113 lines) -14.60% -3.10% -17.08%

aarch64

compressed unpacked .text
this PR -1.30% -1.30% -1.77%
#44 -11.01% +0.01% -13.39%

#44 is worth roughly 10x more compressed, which is what matters for wheel download size, so if you are happy with its complexity it is clearly the better change and this one becomes redundant (it is a subset of #44).

The one place this PR is not simply a weaker version: uncompressed aarch64. #44 trades .text for .data.rel.ro + .rela.dyn, and on aarch64 those cancel almost exactly, so its unpacked size is flat at +0.01%. This PR only deletes instantiations and adds nothing, so its -1.30% is a straight reduction in bytes on disk and in resident pages. If installed footprint matters more than download size, this is currently the better of the two on that arch.

They are not additive - #44 already contains this change.

Comment thread include/mrbind/targets/pybind11/core.h Outdated
/* Comment, if any. */ \
MRBIND_PREPEND_COMMA(comment_) \
/* Comment, possibly null. */ \
, (const char *)DETAIL_MB_PB11_COMMENT_PTR(comment_) \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Move the cast into the macro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants