Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[do not merge] Consider removing isinstance check from trampoline #228

Closed
wants to merge 1 commit into from

Conversation

gatesn
Copy link
Contributor

@gatesn gatesn commented Oct 28, 2023

Pydust performs an isinstance check inside the Trampoline.unwrap function (called from py.as(T, pyobj)) since this provides a nice level of automatic type-checking when invoking functions from Python.

Of course.... runtime type checking has a cost. It's not always desirable, and in the case of the self argument in instance/class methods it's kind of unnecessary.

Currently Pydust types are all heap-allocated (vs static types) to allow us to compile for the limited ABI.

Our isinstance check delegates to the Python ffi.PyObject_IsInstance function, which requires us to give a PyType. We grab this PyType by performing an import of the extension module and grabbing the class object from that. I'm not sure if there's a better way for heap-allocated types? Maybe we insert some global state into our generated libraries that populates on first import? Open to any other ideas?

This seems to address the performance issues in #226 , but ideally I'd like to:

  1. Offer an alternative to py.as when knowingly converting from PyObject to PyDust class struct. Provide fast conversion from PyObject to Pydust type struct #227
  2. Avoid isinstance checks on self/cls parameters (probably using 1)

@gatesn gatesn changed the title Consider removing isinstance check from trampoline [do not merge] Consider removing isinstance check from trampoline Oct 28, 2023
gatesn added a commit that referenced this pull request Oct 30, 2023
TODO(ngates): we should store PyType objects on module state and then
auto-traverse them. See #229

Fixes #226, #227, #228
@lwwmanning lwwmanning closed this Oct 30, 2023
@robert3005 robert3005 deleted the ngates/class-method-perf branch October 30, 2023 17:58
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.

3 participants