Phase 3: Check thread arguments #39
Conversation
TobiasWrigstad
force-pushed
the
cown-arguments-to-threads
branch
from
4826acf to
d6c7f5e
Compare
TobiasWrigstad
force-pushed
the
cown-arguments-to-threads
branch
from
2a1c98d to
5783b7b
Compare
Python/bltinmodule.c
Outdated
| builtin_is_pyrona_program_impl(PyObject *module) | ||
| /*[clinic end generated code: output=2b6729469da00221 input=d36655a3dc34a881]*/ | ||
| { | ||
| return Py_is_invariant_enabled() ? Py_True : Py_False; |
There was a problem hiding this comment.
Does feel like a bit of abuse of notions. I think putting a TODO here, se that we don't merge phase 3 without revisiting. Really I think we want a has used Pyrona, and that is the name of the flag that is used to determine if we should do the region check.
There was a problem hiding this comment.
I fully agree. Will add the TODO as you suggest.
|
I think for this, we should ultimately have a different thread start function that gives us DRF. This can initially be just the same as this, and check the arguments. When we add multi-core, this can then dispatch to a difference core/interpreter, and will have a different local region. Then we would get to a point, where all threads on an interpreter are potentially racing, and across interpreters they aren't. This would give a good migration story. I think this is fine for now, but we should add comments, that we are going to back out these changes, and add a new API call that code should migrate too. |
mjp41
dismissed
their stale review
I think we should revisit this design and possibly have a separate call for DRF thread creation.
Adds a check to thread constructors ensuring that only cowns, immutables, and externally unique regions can be passed in as thread arguments.
TobiasWrigstad
force-pushed
the
cown-arguments-to-threads
branch
from
1328d22 to
7ff6238
Compare
Agreed. I have now created a separate function
👍
I've added a comment on |
TobiasWrigstad
force-pushed
the
cown-arguments-to-threads
branch
from
a5ff8c5 to
4245516
Compare
TobiasWrigstad
force-pushed
the
cown-arguments-to-threads
branch
from
4245516 to
250fa97
Compare
Python/bltinmodule.c
Outdated
| /*[clinic input] | ||
| is_pyrona_program as builtin_is_pyrona_program |
There was a problem hiding this comment.
I this used anymore? Should it still be part of this PR? I think now there is a separate API, we don't need to expose this?
There was a problem hiding this comment.
Good point. I will remove it!
Co-authored-by: Matthew Parkinson <mjp41@users.noreply.github.com>
|
@mjp41 I've addressed your comments including adding +1 to the RCs due to your refactoring (I recall now that I did not make that refactoring to begin with to lower the RC's by one, but I'd already forgotten and someone will make this improvement in the future and get hit so better bite the bullet now). I think this is ready to go in! |
Adds a check to thread constructors ensuring that only cowns, immutables, and externally unique regions can be passed in as thread arguments.
There may be other things to check as well here but this is the MVP consistent with the paper.
Update these checks are now only run if the region invariant is enabled. This is probably a design we want to slightly revisit later.
Update 2 Clinic stuff is now sorted.