Old Program disposal (memory leak) #58138
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/program.ts
Outdated
| } | ||
|
|
||
| // unconditionally set oldProgram to undefined to prevent it from being captured in closure | ||
| createProgramOptions.oldProgram = undefined; |
There was a problem hiding this comment.
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.
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.
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.
they are .. they are used in functions like tryReuseProgram etc..
| 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.
@jakebailey @sheetalkamat What about this then? I admit the previous way to unreference oldProgram was confusing.
There was a problem hiding this comment.
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.
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.
The most helpful thing would be a repro that we can run that shows this leak.
There was a problem hiding this comment.
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.
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.
typescript-bot
added
For Backlog Bug
|
@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. |
|
There's a repro available here now: #58137 (comment) |
|
I posted #61034 which is this PR (I added a co-author for you), plus some changes to make me feel better about the idea of accepting this kind of hack. |
|
Actually, I found a better cause; #61034 now contains a fix for that, which is not too far off from this PR. Going to close this one. |
7 participants
Fixes #58137