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
Fix memory leak issue with dev tools #1335
Conversation
@hehaijin: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
6612b00
to
75942a8
Compare
@@ -18,9 +18,6 @@ export function createPortMessageAdapter< | |||
return { | |||
addListener(listener) { | |||
port.onMessage.addListener(listener); | |||
port.onDisconnect.addListener(() => { |
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 event handler would hold reference to listener and prevent it from garbage collected.
75942a8
to
3f0b6b0
Compare
Hey @hehaijin 👋 Thanks so much for this PR! I don't actually remember why I added the code to remove the In any case, this PR looks good and I think its a small enough change that should be fine. If you wouldn't mind adding a changeset to this PR, I'd be happy to get this merged and released. Appreciate all the digging you've been doing into the memory consumption of the devtools! We'd welcome any improvements you find! |
Thanks @jerelmiller for reviewing. Added changeset. |
@jerelmiller I have some thoughts regarding performance posted in discussion: #1337 |
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.
🎉 Thanks again so much for this PR!
#214 Bundle Size — 851.54KiB (-0.02%).Warning Bundle contains 5 duplicate packages – View duplicate packages Bundle metrics
|
Current #214 |
Baseline #212 |
|
---|---|---|
Initial JS | 814.14KiB (-0.02% ) |
814.34KiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 8.39% |
0% |
Chunks | 5 |
5 |
Assets | 12 |
12 |
Modules | 638 |
638 |
Duplicate Modules | 35 |
35 |
Duplicate Code | 5.2% (-0.38% ) |
5.22% |
Packages | 58 |
58 |
Duplicate Packages | 4 |
4 |
Bundle size by type 1 change
1 improvement
Current #214 |
Baseline #212 |
|
---|---|---|
JS | 814.14KiB (-0.02% ) |
814.34KiB |
IMG | 35.85KiB |
35.85KiB |
HTML | 810B |
810B |
Other | 778B |
778B |
Bundle analysis report Branch hehaijin:fix-leak Project dashboard
This is to fix the memory leak identified in #1332
Each copy of client data to extension is wrapped inside a promise,
and this data is supposed to be removed and GCed but right now it's not.
in src/extension/messageAdapters.ts
createPortMessageAdapter
the line
port.onDisconnect.addListener
createsa closure that holds the reference to
handler
passed in, and only release when disconnect,which may not happen very soon.
And in src/extension/rpc.ts
createRpcClient
function, the the handler holdsa reference to the promise that get data from client side.
so the promise data never got garbage collected.
Removed this event handler from onDisconnect to fix the memory leak issue.
May need to address how to properly remove it in
onDisconnect
, but it's lower priority.closes #1332