Synchronize mutable env in functions #1625
Conversation
MarkMcCaskey
force-pushed
the
fix/synchronize-mutable-env
branch
from
5e10ec6 to
514ccf8
Compare
|
bors try |
tryBuild succeeded: |
Codecov Report
@@ Coverage Diff @@
## master #1625 +/- ##
==========================================
- Coverage 31.75% 31.68% -0.08%
==========================================
Files 185 185
Lines 27034 27118 +84
==========================================
+ Hits 8584 8591 +7
- Misses 18450 18527 +77
Continue to review full report at Codecov.
|
| // TODO: the `&mut *self.env.as_ptr()` is likely invoking some "mild" | ||
| // undefined behavior due to how it's used in the static fn call | ||
| unsafe { (*self.func)(&mut *self.env.as_ptr(), &args) } | ||
| let mut env_guard = self.env.lock().unwrap(); |
There was a problem hiding this comment.
I'm not really a fan of this. This is a clear performance penalty that we are now incurring for all function calls.
| let func: &Func = unsafe { &*(&() as *const () as *const Func) }; | ||
| let mut env_guard = env.lock().unwrap(); |
There was a problem hiding this comment.
I'm not really a fan of this. This is a clear performance penalty that we are now incurring for all function calls.
There was a problem hiding this comment.
We should measure this penalty, because I don't see a lot more options to solve this sync problem.
There was a problem hiding this comment.
I'm completely in favor of measurement, but I'm also sympathetic to wanting a fast path for singlethreaded code which will likely be the majority of uses (at least in the short-term).
Also, I agree that yeah, this change is nice because it just works without much changes. I wrote up a design doc about solutions to this problem and I'll ping people about it next week: I think it's probable that we can make a more complex solution to this problem, but it will require breaking changes to our API or duplicating our API (singlethreaded version vs general version). I also talked about some more exotic options with Nick in regards to things like updating vtables at runtime to dynamically synchronize code when it becomes multithreaded but I don't think we're realistically going to be trying anything fancy any time soon and also I haven't thought of any fancy solutions that aren't a bad trade off in terms of complexity.
|
This PR is not being actively developed, we've decided to pursue #1663 instead |
1663: Make Function Env immutable r=MarkMcCaskey a=MarkMcCaskey Alternative / related to #1625 Resolves #1612 # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Mark McCaskey <[email protected]> Co-authored-by: Mark McCaskey <[email protected]>
Resolves #1612
This PR wraps all
Envs instd::sync::Mutexensuring that mutable access cannot happen concurrently. This PR also fixes some undefined behavior / broken code that was accidentally working withVMDynamicFunctionWithEnv.As a next step, we should design new APIs to give users more control over
Env, by for example having anenvandenv_mutvariation where theenvvariation passes a&Envallowing users to synchronize themselves if they want. Additionally, it may be valuable to add unsafe versions of these functions for power users of the API. Finally, we should investigate if it's possible to allow direct&mutaccess in a single threaded context with a sound API (by using trait constraints).Review