Skip to content

fix: fix Compiler not getting freed after use#8268

Merged
h-a-n-a merged 4 commits intomainfrom
memory-leak
Nov 12, 2024
Merged

fix: fix Compiler not getting freed after use#8268
h-a-n-a merged 4 commits intomainfrom
memory-leak

Conversation

@h-a-n-a
Copy link
Copy Markdown
Contributor

@h-a-n-a h-a-n-a commented Oct 30, 2024 •
edited
Loading

Summary

closes #3169

Fixed an issue when constantly creating compilers and the memory is not getting freed after use.

This issue was fixed with WeakRef and function expression which does not capture this, which is Compiler in this case. But still, I'm not able to create a MRE with both minimal Rust and JS code. So this issue might be appropriately tackled once a MRE is created.

Tests will be added in up-coming PRs. We might need to take https://github.com/napi-rs/napi-rs/tree/main/memory-testing as a reference.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@github-actions github-actions bot added release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack. labels Oct 30, 2024
@netlify
Copy link
Copy Markdown

netlify bot commented Oct 30, 2024 •
edited
Loading

✅ Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit dd57bc3
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/6732d029f520a20008b2756c

@h-a-n-a
Copy link
Copy Markdown
Contributor Author

h-a-n-a commented Oct 31, 2024

Release Canary
Version: 1.0.15-canary-cc53546f-20241030081506

@h-a-n-a h-a-n-a force-pushed the memory-leak branch 3 times, most recently from 15d1368 to e314c9b Compare November 7, 2024 06:53
h-a-n-a
h-a-n-a commented Nov 12, 2024
View reviewed changes
@h-a-n-a h-a-n-a requested review from SyMind and ahabhgk November 12, 2024 05:35
@h-a-n-a h-a-n-a marked this pull request as ready for review November 12, 2024 05:35
@h-a-n-a h-a-n-a changed the title fix: memory leak in JavaScript Compiler fix: fix Compiler not getting freed after use Nov 12, 2024
@h-a-n-a h-a-n-a merged commit 8f75992 into main Nov 12, 2024
@h-a-n-a h-a-n-a deleted the memory-leak branch November 12, 2024 05:41
@dinofx
Copy link
Copy Markdown

dinofx commented Feb 20, 2025

This approach seems to have quite a negative performance impact:

image

Above only captured the place where const getCompiler = () => ref.deref()! was called. The places where const that = new WeakRef(this) pattern was used doesn't get measured, so there's likely additional impact not shown.

@dinofx
Copy link
Copy Markdown

dinofx commented Feb 20, 2025

@h-a-n-a
I'm guessing that #updateNonSkippableRegisters() is accessing most of these weak refs. If that's the case, it looks like close to half a second is being spent dereferencing these. That's a pretty significant percentage of our total build time.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahabhgk ahabhgk ahabhgk approved these changes

@SyMind SyMind Awaiting requested review from SyMind

Assignees

@h-a-n-a h-a-n-a

Labels

release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Memory leak when using @rspack/core API

3 participants

@h-a-n-a @dinofx @ahabhgk