Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

use dynamic trampoline for all getters and setters #3029

Merged
merged 1 commit into from
May 9, 2023

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Mar 9, 2023

This is an extension to the "trampoline" changes made in #2705 to re-use a single trampoline for all #[getter]s (and similar for all #[setters]). It works by setting the currently-unused closure member of the ffi::PyGetSetDef structure to point at a new struct GetSetDefClosure which contains function pointers to the getter / setter implementations.

A universal trampoline for all getter, for example, then works by reading the actual getter implementation out of the GetSetDefClosure.

Advantages of doing this:

  • Very minimal simplification to the macro code / generated code size. It made a 4.4% reduction to test_getter_setter generated size, which is an exaggerated result as most code will probably have lots of bulk that isn't just the macro code.

Disadvantages:

  • Additional level of complexity in the getter and setter trampolines and accompanying code.
  • To keep the GetSetDefClosure objects alive, I've added them to the static LazyTypeObject inner.
  • Very slight performance overhead at runtime (shouldn't be more than an additional pointer read). It's so slight I couldn't measure it.

Overall I'm happy to either merge or close this based on what reviewers think!

@davidhewitt davidhewitt marked this pull request as draft March 10, 2023 16:09
@davidhewitt davidhewitt force-pushed the trampoline-getter-setter-2 branch from 43d7554 to 0acca88 Compare March 10, 2023 19:07
@davidhewitt davidhewitt force-pushed the trampoline-getter-setter-2 branch from 0acca88 to aeb7c29 Compare May 9, 2023 07:04
@davidhewitt davidhewitt force-pushed the trampoline-getter-setter-2 branch from aeb7c29 to 1ea57cb Compare May 9, 2023 07:10
@davidhewitt davidhewitt marked this pull request as ready for review May 9, 2023 07:10
@davidhewitt
Copy link
Member Author

Ok, I've pushed a commit which removes the use of unreachable_unchecked. It does this by moving the logic constructing the getters & setters into a builder / enum in create_type_object.rs, and picking an appropriate trampoline / closure pair inside match arms.

I've added some TODO where we could add some validation that only one #[getter] per property etc, which can be a future PR.

@davidhewitt
Copy link
Member Author

  • To keep the GetSetDefClosure objects alive, I've added them to the static LazyTypeObject inner.

I think this is no longer a disadvantage, as I've added the "name" and "doc" members to this too, which addresses an older TODO to avoid naively leaking those things.

@adamreichold
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented May 9, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit c27a633 into PyO3:main May 9, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants