Skip to content
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

Don't run GC on isolate disposal #1181

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

bartlomieju
Copy link
Member

This commit removes triggering "low memory notification" during isolate
disposal. We were getting segfaults in Deno due to this notification. The root cause
of the segfault is still unknown and reproducing the segfault is not easy - I wasn't
able to reproduce it once on Apple M1, I was able to reproduce it on Windows by
running a test suite in a loop for a few minutes.

Removing this garbage collection trigger removes the guarantee that
"regular" FinalizerCallbacks will be called before the isolate goes away.
It is fine as both spec and V8 do not provide this guarantee and we were
overly strict in this case.

CC @andreubotella

@ry
Copy link
Member

ry commented Feb 2, 2023

  1. Randomly removing bits doesn’t seem like a solid fix. We can just delay upgrading V8 until we have a proper fix.

  2. I don’t know why we would want to trigger GC on isolate deallocation anyway. Have you researched why it was added in the first place?

@bartlomieju
Copy link
Member Author

bartlomieju commented Feb 2, 2023

  1. Randomly removing bits doesn’t seem like a solid fix. We can just delay upgrading V8 until we have a proper fix.

I agree, but I hasn't been able to pinpoint the exact conditions the segfault happens at. We knew there's a problem somewhere in the disposal logic, because we get sporadic segfaults on Windows in deno. I spoke with Bert about this problem but we haven't had a chance to address it yet.

  1. I don’t know why we would want to trigger GC on isolate deallocation anyway. Have you researched why it was added in the first place?

I spoke with Andreu who implemented this and it turns out this was a temporary solution that is not strictly required: https://discord.com/channels/684898665143206084/713035341736706089/1070469717803937875.

@andreubotella
Copy link
Contributor

andreubotella commented Feb 2, 2023

I spoke with Andreu who implemented this and it turns out this was a temporary solution that is not strictly required: https://discord.com/channels/684898665143206084/713035341736706089/1070469717803937875.

In order to achieve certain properties that were needed to clean up context slots and some other N-API stuff, in #895 I decided to trigger GC on isolate drop to have a guarantee that finalizers would run unless they are reachable from a v8::Global at isolate drop time. This turned out to not be enough for those use cases, which is why in #1076 I added finalizers that are guaranteed to run, even if that is after the isolate has been disposed. I now think finalizers which must run at some point, whether that is for correctness or to avoid memory leaks, should be using guaranteed finalizers, and so we should probably get rid of this extra guarantee on regular finalizers.

andreubotella added a commit to andreubotella/rusty_v8 that referenced this pull request Feb 2, 2023
This change updates the documentation of `Weak::with_finalizer` to
denoland#1181. It also adds documentation to `Weak::with_guaranteed_finalizer`
reflect the fact that the previous stricter guarantees were dropped in
and `Weak::clone_with_guaranteed_finalizer`.
andreubotella added a commit to andreubotella/rusty_v8 that referenced this pull request Feb 2, 2023
This change updates the documentation of `Weak::with_finalizer` to
reflect the fact that the previous stricter guarantees were dropped in
denoland#1181. It also adds documentation to `Weak::with_guaranteed_finalizer`
and `Weak::clone_with_guaranteed_finalizer`.
@bartlomieju bartlomieju merged commit d3fff51 into denoland:main Feb 2, 2023
@bartlomieju bartlomieju deleted the dont_gc_on_dispose branch February 2, 2023 10:59
bartlomieju pushed a commit that referenced this pull request Feb 2, 2023
This change updates the documentation of `Weak::with_finalizer` to
reflect the fact that the previous stricter guarantees were dropped in
#1181. It also adds documentation to `Weak::with_guaranteed_finalizer`
and `Weak::clone_with_guaranteed_finalizer`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants