A safer core: remove Obj.magic in promise state update
#1084
Conversation
This commit guarantees that, even if we later weaken the type information available on `state`, we keep precise guarantees on `result` values -- they are not a subset of possible states anymore, but a separate type. There may be a performance impact to this change, so running benchmarks would be useful.
|
Some of the static discipline in the code is moved around instead of removed entirely.
Point (2) may have an impact on performance. (I think that it will be neglectible, but I don't understand which parts of the Lwt core are performance-critical so I am not sure.) Do you know how to run benchmarks to measure the performance impact of this PR? |
The static discipline on promise states requires an `Obj.magic` in promise state update. This appears to work correctly under OCaml 4, but is known to cause segfaults under concurrent usage with OCaml 5.
|
thanks for the contribution :) re: perf & bench The part that needs to be real cheap is the non-cooperating binds. That is the binds (and maps and such) where the promise is already resolved and even more so the binds where the promise is already resolved and the function returns an already resolved promise. These correspond to the part of the program where the user code doesn't do any I/O. |
| | Error _ as p_result -> | ||
| let State_may_now_be_pending_proxy p'' = may_now_be_proxy p'' in | ||
| let p'' = underlying p'' in | ||
| let _state, p'' = underlying p'' in |
There was a problem hiding this comment.
Self-review note: this could be let p'' = underlying_promise p'' (same for the other occurrence of let _state, ... = underlying .. below).
|
I understand from #1079 (comment) that this PR proved difficult to review. I wondered if it's worth having if multi-domain gets temporarily removed from Lwt. I have mixed opinions on this:
|
2 participants
This is a proposal to fix #1079. I removed the fine-grained typing of the mutable state of a promise, so that the unsafe casts that were previously used are unnecessary. This should remove the crash observed under multicore usage.