-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[browser][coreCLR] finalizer on a browser event loop #123629
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
base: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @agocke, @dotnet/gc |
| SString err; | ||
| ex->GetMessage(err); | ||
| LogErrorToHost("Error message: %s", err.GetUTF8()); | ||
| abort(); |
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 don't think calling abort() here is what we want. See FinalizerThread::FinalizerThreadStart where we will continue to loop.
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 thought that we are supposed to "kill the process" when we see managed exception in the finalizer thread.
On other OS FinalizerThread::FinalizerThreadStart is running on different(main) thread than the finalizer.
How does the UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP know that something failed on finalizer thread ?
I can use CrashDumpAndTerminateProcess(1); that is there in the macro instead of the abort()
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.
when we see managed exception in the finalizer thread.
That is mostly true, but there are configurations here that we should only change with a clear comment on why. There is a fair bit of native infrastructure here that could trigger and we should be sensitive to that. Take a look at ManagedThreadBase_DispatchOuter and ManagedThreadBase_DispatchMiddle. @jkotas or @janvorli Can probably educate us both on the precise expectations here and what is reasonable to do.
I can use CrashDumpAndTerminateProcess(1); that is there in the macro instead of the abort()
That is a better place to start and what I would expect that API instead of abort() regardless of what we do. I might suggest CrashDumpAndTerminateProcess(ex->GetHR());.
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.
How does the UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP know that something failed on finalizer thread ?
I am not sure if I understand the question correctly. When a managed exception escapes the finalizer thread code, it is actually in a form of PAL_SEHException, so that macro catches it, reports the exception as unhandled and then calls the CrashDumpAndTerminateProcess to shutdown the runtime.
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've forgotten to mention that this is for Unix. On Windows, it is a SEH exception and the OS itself takes care of it (shutting down the process, generating dump)
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 UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP is close to what we want for wasm.
You may want to actually test what happens when you throw in the finalizer on wasm. You should see unhandled exception message with stacktrace.
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 see my mistake now. I got confused by ManagedThreadBase::KickOff. I thought it's creating another thread. I will continue tomorrow. Thank you so far!
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
|
|
||
| public static bool IsPreciseGcSupported => !IsMonoRuntime | ||
| && !IsBrowser; // TODO-WASM: https://github.com/dotnet/runtime/issues/114096 | ||
| public static bool IsPreciseGcSupported => !IsMonoRuntime; |
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.
@janvorli Are we actually running successfully all tests assuming precise GC ? I know we are reporting quite a fair chunk of vars conservatively (I believe everything aside from the IL globals, args and locals)
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.
IIRC, @davidwrighton has enabled conservative reporting of call arguments and temp vars, the rest should be reported precisely. David, have I forgotten about something?
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 believe that the conservative reporting in the interpreter is still "precise" - for the definition of "precise" used by this property.
Precise GC here means that the runtime is not trying to interpret random values found on the stack as GC object references.
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.
We are not running with fully precise GC today. I had an idea about how we could implement it semi-efficiently, but I haven't actually tried to see if it would work. However, the level of conservative GC we have today is extremely limited relative to what Mono was doing, so we might be close enough to precise GC semantics for nearly all the tests to pass. The notable detail is that I believe we are only reliably visibly imprecise for tests which check to see if a value got promoted (since our reporting will pin things that don't need pinning), and during the instruction after a call instruction. This is a MUCH smaller problem set than Mono had.
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.
The notable detail is that I believe we are only reliably visibly imprecise for tests which check to see if a value got promoted (since our reporting will pin things that don't need pinning)
This sounds like we are just extending a lifetime within a method, correct? That does would count as imprecise GC. RyuJIT extends the lifetime within a method as well.
| // Wait for work to do... | ||
|
|
||
| _ASSERTE(GetFinalizerThread()->PreemptiveGCDisabled()); | ||
| #ifdef _DEBUG |
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.
These GetFinalizerThread()->m_GCOnTransitionsOK = FALSE/TRUE should be moved up as well. It is important that the call to EnablePreemptiveGC is enclosed by these calls - it is a perf optimization for GC stress.
| GCHeapUtilities::GetGCHeap()->CollectionCount(GCHeapUtilities::GetGCHeap()->GetMaxGeneration()); | ||
| FinalizeAllObjects(); | ||
|
|
||
| GetFinalizerThread()->DisablePreemptiveGC(); |
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.
Should this be after the ifdef?
| } | ||
|
|
||
| VOID FinalizerThread::FinalizerThreadWorkerIteration() | ||
| { |
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.
Could you please add the expected contract here? (maybe you have not pushed your local changes up yet)
Fixes #114096