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

Decide whether to lock function arguments at compile time #720

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

oremanj
Copy link
Contributor

@oremanj oremanj commented Sep 16, 2024

An attempt at the static locking determination that I suggested in #695 (comment). Note this PR is against the free-threaded branch.

Behavior differences from the free-threaded branch without this change:

  • it is no longer possible to do nb::arg().lock(false) or .lock(runtime_determined_value); this could be re-added by restoring cast_flags::lock and checking the arg_flags at runtime, but I didn't think it was worth the complexity
  • we no longer prohibit locking self in __init__; changing this would also require restoring cast_flags::lock, and it's not clear what benefit it would have (sure the lock is somewhat superfluous but do we really care?)

@wjakob
Copy link
Owner

wjakob commented Sep 18, 2024

This is great! It is certainly better than my runtime solution. Two thoughts:

  1. You had mentioned in the prior PR that it's unfortunate if nb::arg().lock requires the complex function dispatch loop. Does this adress that Issue? I am thinking that some runtime check in nb_func_new is still needed to discover that the simple dispatch base case still applies despite argument annotations being present.

  2. All the duplication between arg, arg_v, arg_locked, and arg_locked_v seems a bit much with this further compile-time distinction. Would it be better to have single a template class?

template <bool IsLocked, bool HasValue> struct arg {
    const char *name = nullptr;
    PyObject *value = nullptr;
    // ...

   arg</* IsLocked = */ true, HasValue> lock() { return { name, value, ... }; }
   template <typename T>
   arg<IsLocked, /* HasValue = */ true> operator=(T&&x) {... }
};

All the code processing the various args could then match on the arguments.

@wjakob
Copy link
Owner

wjakob commented Sep 18, 2024

After thinking more about it, perhaps having non-template arg variants is also better. This class is used very often in bindings, and having a template here might affect compile time costs. I am not sure.

Copy link
Owner

@wjakob wjakob left a comment

Choose a reason for hiding this comment

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

OOps, I forgot to send off these comments.

include/nanobind/nb_func.h Outdated Show resolved Hide resolved
include/nanobind/nb_func.h Outdated Show resolved Hide resolved
include/nanobind/nb_func.h Outdated Show resolved Hide resolved
@oremanj
Copy link
Contributor Author

oremanj commented Sep 18, 2024

You had mentioned in the prior PR that it's unfortunate if nb::arg().lock requires the complex function dispatch loop. Does this adress that Issue? I am thinking that some runtime check in nb_func_new is still needed to discover that the simple dispatch base case still applies despite argument annotations being present.

Correct, that's separate. This PR makes that improvement possible, but doesn't implement it.

All the duplication between arg, arg_v, arg_locked, and arg_locked_v seems a bit much with this further compile-time distinction. Would it be better to have single a template class?

If we had another axis of variation I would definitely want to go for the template. I think the current case is borderline, and don't mind changing it if you prefer the template, but I think the duplication still winds up a little easier to understand. For backcompat we would need to keep the arg and arg_v names regardless, so the template would be something like arg_t with arg/arg_v as aliases for particular instantiations; then we'd need a metafunction to detect "any instantiation of arg_t", etc.

@wjakob wjakob merged commit 7ccb5e5 into wjakob:free-threaded Sep 18, 2024
26 checks passed
@wjakob
Copy link
Owner

wjakob commented Sep 18, 2024

Thanks a lot!

wjakob pushed a commit that referenced this pull request Sep 18, 2024
This commit refactors argument the locking locking so that it occurs at
compile-time without imposing runtime overheads. The change applies to
free-threaded extensions.

Behavior differences compared to the prior approach:

- it is no longer possible to do ``nb::arg().lock(false)`` or
  ``.lock(runtime_determined_value)``

- we no longer prohibit locking self in ``__init__``; changing this
  would also require restoring ``cast_flags::lock``, and it's not clear
  that the benefit outweighs the complexity.
wjakob pushed a commit that referenced this pull request Sep 19, 2024
This commit refactors argument the locking locking so that it occurs at
compile-time without imposing runtime overheads. The change applies to
free-threaded extensions.

Behavior differences compared to the prior approach:

- it is no longer possible to do ``nb::arg().lock(false)`` or
  ``.lock(runtime_determined_value)``

- we no longer prohibit locking self in ``__init__``; changing this
  would also require restoring ``cast_flags::lock``, and it's not clear
  that the benefit outweighs the complexity.
wjakob pushed a commit that referenced this pull request Sep 20, 2024
This commit refactors argument the locking locking so that it occurs at
compile-time without imposing runtime overheads. The change applies to
free-threaded extensions.

Behavior differences compared to the prior approach:

- it is no longer possible to do ``nb::arg().lock(false)`` or
  ``.lock(runtime_determined_value)``

- we no longer prohibit locking self in ``__init__``; changing this
  would also require restoring ``cast_flags::lock``, and it's not clear
  that the benefit outweighs the complexity.
wjakob pushed a commit that referenced this pull request Sep 20, 2024
This commit refactors argument the locking locking so that it occurs at
compile-time without imposing runtime overheads. The change applies to
free-threaded extensions.

Behavior differences compared to the prior approach:

- it is no longer possible to do ``nb::arg().lock(false)`` or
  ``.lock(runtime_determined_value)``

- we no longer prohibit locking self in ``__init__``; changing this
  would also require restoring ``cast_flags::lock``, and it's not clear
  that the benefit outweighs the complexity.
wjakob pushed a commit that referenced this pull request Sep 20, 2024
This commit refactors argument the locking locking so that it occurs at
compile-time without imposing runtime overheads. The change applies to
free-threaded extensions.

Behavior differences compared to the prior approach:

- it is no longer possible to do ``nb::arg().lock(false)`` or
  ``.lock(runtime_determined_value)``

- we no longer prohibit locking self in ``__init__``; changing this
  would also require restoring ``cast_flags::lock``, and it's not clear
  that the benefit outweighs the complexity.
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