-
Notifications
You must be signed in to change notification settings - Fork 208
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
Conversation
70622d1
to
8155d6c
Compare
70a9493
to
20ae577
Compare
20ae577
to
0ad9042
Compare
c3b0558
to
0b1feea
Compare
This is great! It is certainly better than my runtime solution. Two thoughts:
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 |
After thinking more about it, perhaps having non-template |
There was a problem hiding this 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.
Correct, that's separate. This PR makes that improvement possible, but doesn't implement it.
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 |
82f76aa
to
0bbb9dc
Compare
0bbb9dc
to
babe47d
Compare
Thanks a lot! |
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.
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.
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.
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.
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.
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:
nb::arg().lock(false)
or.lock(runtime_determined_value)
; this could be re-added by restoringcast_flags::lock
and checking thearg_flags
at runtime, but I didn't think it was worth the complexity__init__
; changing this would also require restoringcast_flags::lock
, and it's not clear what benefit it would have (sure the lock is somewhat superfluous but do we really care?)