pyo3 icon indicating copy to clipboard operation
pyo3 copied to clipboard

Reduce the size of `FnArg::Regular` variant (macro-backends)

Open Tpt opened this issue 8 months ago • 4 comments

The RegularArg struct used in FnArg::Regular variant of the macro-backends crate is quite large, triggering the the clippy::large_enum_variant lint. It might be nice to reduce it by using some references.

Tpt avatar Apr 03 '25 07:04 Tpt

Hi all! I'd like to contribute by taking up this refactor if it's available. Are we okay with Box-ing the fields in RegularArg to reduce its size, or would you prefer sticking to references for optimization? Let me know your thoughts, thanks!

jmin-pingu avatar Apr 14 '25 01:04 jmin-pingu

Boxing it is only worth it if it only rarely happens to be the big variant (like say if you have a big error type). RegularArg is pretty common, so it's likely not worth it.

mejrs avatar Apr 14 '25 09:04 mejrs

At the suggestion of @mejrs on Discord, I'm planning to try to fix this during a live coding session with some people working on learning Rust at the Recurse Center.

Here's the output from cargo +nightly rustc -- -Zprint-type-sizes in the macros-backend crate for the RegularArg struct:

print-type-size type: `method::RegularArg<'_>`: 272 bytes, alignment: 8 bytes
print-type-size     field `.from_py_with`: 96 bytes
print-type-size     field `.default_value`: 136 bytes
print-type-size     field `.name`: 24 bytes
print-type-size     field `.ty`: 8 bytes
print-type-size     field `.option_wrapped_type`: 8 bytes

So probably making from_py_with and default_value references will fix this.

ngoldbaum avatar Apr 22 '25 16:04 ngoldbaum

I tried but didn't get terribly far. I got stuck trying to make PyFunctionArgPyO3Attributes wrap references. That may not be necessary though.

ngoldbaum avatar Apr 22 '25 18:04 ngoldbaum

I think this is no longer an issue today. At least I cannot reproduce it. The following command doesn't give any warnings.

❯ cargo clippy -p pyo3-macros-backend  --all-targets

Versions I use:

❯ rustc --version    
rustc 1.83.0 (90b35a623 2024-11-26)

❯ cargo --version
cargo 1.83.0 (5ffbef321 2024-10-29)

❯ cargo clippy --version
clippy 0.1.83 (90b35a6 2024-11-26)

anilbey avatar Jul 19 '25 15:07 anilbey

Full log of running nox-s clippy-all (without the deny warnings flag).

❯ nox -s clippy-all                                                                                   ─╯
nox > Running session clippy-all
nox > CPython 3.7
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.8
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.9
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.10
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.11
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.12
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.13
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.14
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.13t
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > CPython 3.14t
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > PyPy 3.9
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > PyPy 3.10
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > PyPy 3.11
nox > cargo clippy --no-default-features --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3 --all-targets --workspace
nox > cargo clippy --no-default-features --features=full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --no-default-features --features=abi3,full,multiple-pymethods --all-targets --workspace
nox > cargo clippy --manifest-path=pyo3-benches/Cargo.toml
nox > cargo doc --manifest-path=pyo3-ffi-check/Cargo.toml -p pyo3-ffi --no-deps
nox > cargo clippy --manifest-path=pyo3-ffi-check/Cargo.toml --workspace --all-targets
nox > Session clippy-all was successful.

anilbey avatar Jul 19 '25 15:07 anilbey

You'll need to remove the #[allow] attribute to get clippy to report it:

https://github.com/PyO3/pyo3/blob/ecf59e0b7f0ba447004ffa7541bf7e79febb2c20/pyo3-macros-backend/src/method.rs#L85

davidhewitt avatar Jul 20 '25 12:07 davidhewitt

I did

anilbey avatar Jul 21 '25 09:07 anilbey

This definitely still triggers for me if I remove that line:

david@david-pc:~/dev/pyo3$ nox -s clippy
nox > Running session clippy
nox > cargo clippy --no-default-features --all-targets --workspace -- --deny=warnings
    Checking linux-raw-sys v0.9.4
   Compiling pyo3-build-config v0.25.1 (/home/david/dev/pyo3/pyo3-build-config)
    Checking getrandom v0.3.3
    Checking phf_shared v0.12.1
    Checking proptest v1.6.0
    Checking rayon v1.10.0
    Checking pyo3-introspection v0.25.1 (/home/david/dev/pyo3/pyo3-introspection)
    Checking phf v0.12.1
    Checking uuid v1.17.0
    Checking chrono-tz v0.10.4
    Checking rustix v1.0.8
    Checking tempfile v3.20.0
   Compiling pyo3-macros-backend v0.25.1 (/home/david/dev/pyo3/pyo3-macros-backend)
   Compiling pyo3-ffi v0.25.1 (/home/david/dev/pyo3/pyo3-ffi)
   Compiling pyo3 v0.26.0-dev (/home/david/dev/pyo3)
   Compiling pyo3-pytests v0.1.0 (/home/david/dev/pyo3/pytests)
error: large size difference between variants
  --> pyo3-macros-backend/src/method.rs:86:1
   |
86 | / pub enum FnArg<'a> {
87 | |     Regular(RegularArg<'a>),
   | |     ----------------------- the largest variant contains at least 328 bytes
88 | |     VarArgs(VarargsArg<'a>),
   | |     ----------------------- the second-largest variant contains at least 32 bytes
89 | |     KwArgs(KwargsArg<'a>),
90 | |     Py(PyArg<'a>),
91 | |     CancelHandle(CancelHandleArg<'a>),
92 | | }
   | |_^ the entire enum is at least 328 bytes
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#large_enum_variant
   = note: `-D clippy::large-enum-variant` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(clippy::large_enum_variant)]`
help: consider boxing the large fields to reduce the total size of the enum
   |
87 -     Regular(RegularArg<'a>),
87 +     Regular(Box<RegularArg<'a>>),
   |

davidhewitt avatar Jul 25 '25 13:07 davidhewitt