-
Notifications
You must be signed in to change notification settings - Fork 12.3k
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
Old Program disposal (memory leak) #58138
base: main
Are you sure you want to change the base?
Conversation
src/compiler/program.ts
Outdated
@@ -1858,6 +1858,7 @@ export function createProgram(rootNamesOrOptions: readonly string[] | CreateProg | |||
} | |||
|
|||
// unconditionally set oldProgram to undefined to prevent it from being captured in closure | |||
createProgramOptions.oldProgram = undefined; |
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.
This is modifying an input, so isn't a good idea.
If you wanted to ensure it was gone, it would be better to turn it into a let
and then set that to undefined, just like the variables below.
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 am also wondering why this should cause the leak as unlike oldProgram and other things, this isnt used in any other closures and only at the start of the functions so technically this shouldnt cause issue? Am i missing 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.
Well, the other variables below this are also not used within closures (as far as I can tell), so there's some funkiness here for sure which may imply that "not using" is not enough to not close over it, but to fix that should mean to do exactly what this code already does to fix that problem.
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.
they are .. they are used in functions like tryReuseProgram etc..
@@ -1801,6 +1801,9 @@ export function createLanguageService( | |||
}; | |||
program = createProgram(options); | |||
|
|||
// unconditionally set oldProgram to undefined to prevent it from being captured in closure | |||
options.oldProgram = undefined; |
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.
@jakebailey @sheetalkamat What about this then? I admit the previous way to unreference oldProgram was confusing.
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.
This makes less sense... The thing I suggested (doing the same thing that the existing code did where you last edited) seems better.
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.
Ok I get it.
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.
Ok I played around with different approaches and could never figure out a clean way to fix this leak. I feel like there's indeed some funkiness in JSC that I just don't comprehend...
I will keep my fork for now since I cannot go forward without this ugly fix, but will try to comeback here if ever an update seems to fix anything...
I'm open to any other suggestion
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 most helpful thing would be a repro that we can run that shows this leak.
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.
JSC is technically a conservative collector, so that may tie into things. That said, it's also possible there's a leak for a precise GC without this PR.
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 will build a little something to make the issue reproducible and figure out what could go on. But I will need some time. Please allow me 1-2 week and I will come back here with a clear workable playground.
@sheetalkamat or @jakebailey do you think the fix for #58137 is likely to be close to this PR? If so, let's keep it open. If not, let's close it and wait until one of us can get the repro to work for us. (Either way, one of the reviewers needs to repro locally to find out if this PR works.) |
#58138 is this PR, did you mean something else? |
Oops, yes, I meant #58137, the issue it's fixing. |
If we have a repro, I can imagine there being a fix, but this change itself still looks incorrect. I believe we are waiting for some kind of repro. |
Fixes #58137