-
Notifications
You must be signed in to change notification settings - Fork 786
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
AsyncMemoize: do not cancel and restart jobs that are being requested #16547
Conversation
❗ Release notes required
|
Wow, so MacOS and Linux passes 😅. The one failure seems unrelated, and due to concurrent access to
We should probably add some locks there. |
@0101, if this is checks green, please take a look if this is the right direction. |
The idea behind this is that the computation will run directly in the first requestor's thread and therefore will retain the stack trace. Which would be more valuable than saving the occasional restarts. Unfortunately as it is now, the computation still gets disconnected. However it's still my ambition to make the stack traces work. It might be nice to have this configurable and have stack traces for debugging and no-restarts for release. |
Debugging as in debugging the FCS itself? |
Yep. Sometimes it would be nice to know when an exception happens what was the whole path to it. Now you just get the job where it happened being fired off form the thread pool. |
Hmmm ok, so this passes without reverting16348 (#16536). |
Damn, how come? The failures that happen in the other branch don't even touch this code 🤔 |
tbh, I now think this code just hides the problem better. |
let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger | ||
let phase = DiagnosticsThreadStatics.BuildPhase | ||
let ct2 = Cancellable.Token | ||
|
||
try | ||
let work = | ||
async { | ||
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger | ||
DiagnosticsThreadStatics.BuildPhase <- phase | ||
Cancellable.Token <- ct2 | ||
return! computation |> Async.AwaitNodeCode | ||
} | ||
|
||
Async.StartAsTask(work, cancellationToken = defaultArg ct CancellationToken.None) | ||
finally | ||
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger | ||
DiagnosticsThreadStatics.BuildPhase <- phase | ||
Cancellable.Token <- ct2 | ||
Async.StartAsTask(computation |> Async.AwaitNodeCode, ?cancellationToken = ct) |
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.
Removing these bits looks suspicious to me. It's what keeps the needed parts of the compiler context when it switches threads.
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.
I think I just replaced it with what we use in production? (AwaitNodeCode)
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.
Now that I look at it, this code is different than wrapThreadStaticInfo
.
Here DiagnosticsThreadStatics
of the calling thread are passed onto the new thread.
In wrapThreadStaticInfo
they are not:
fsharp/src/Compiler/Facilities/BuildGraph.fs
Lines 16 to 26 in 3af3d41
let wrapThreadStaticInfo computation = | |
async { | |
let diagnosticsLogger = DiagnosticsThreadStatics.DiagnosticsLogger | |
let phase = DiagnosticsThreadStatics.BuildPhase | |
try | |
return! computation | |
finally | |
DiagnosticsThreadStatics.DiagnosticsLogger <- diagnosticsLogger | |
DiagnosticsThreadStatics.BuildPhase <- phase | |
} |
At this moment this is just trying things in the dark. I don't think it fixes anything. |
This utilizes changes from #16533
The idea is to never cancel a job that is awaited by someone.
Basically cancelling a
Get
does not cancel the cached computation if there are other requests so there is no need for restarts.I removed Linked CTS so computations can only be canceled by posting a message to
processStateUpdate
.Disclaimer: I'm probably missing something and don't understand how this is expected to work, but just wanted to see it this passes, because it's green locally.