tls_wrap: Unlink TLSWrap and SecureContext objects asap. #1580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Just tested — the same results are applicable to the server — a full gc run goes from ~30ms to ~14ms. |
|
Full stack trace: Looking into it... |
indutny
added a commit
to indutny/io.js
that referenced
this pull request
May 2, 2015
`StreamBase::AfterWrite` is passing handle as an argument to the `afterWrite` function in net.js. Thus GC should not collect the handle and the request separately and assume that they are tied together. With this commit - request will always outlive the StreamBase instance, helping us survive the GC pass. Fix: nodejs#1580
|
Should be fixed by #1590 |
|
Yes, that issue is fixed by #1590 for me. It didn't affect the GC timings I posted above. |
indutny
added a commit
that referenced
this pull request
May 3, 2015
`StreamBase::AfterWrite` is passing handle as an argument to the `afterWrite` function in net.js. Thus GC should not collect the handle and the request separately and assume that they are tied together. With this commit - request will always outlive the StreamBase instance, helping us survive the GC pass. Same applies to the ShutdownWrap instances, they should never be collected after the StreamBase instance. Fix: #1580 PR-URL: #1590 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
ChALkeR
force-pushed
the
unlink-tlswrap-securecontext
branch
from
7fa24d6
to
0bc9ec8
Compare
|
Rebased against current master (with #1590 fix), the timings are about the same as above so I won't copy them here again. |
|
Is there any reason to use I wonder that it deletes properties defined in a constructor so that it would break a reference to the hidden class. Substituting master this PR PR without using delete. diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index c0edd0e..0a30cd3 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -302,9 +302,9 @@ TLSSocket.prototype._destroySSL = function _destroySSL() {
this.ssl.destroySSL();
if (this.ssl._secureContext.singleUse) {
this.ssl._secureContext.context.close();
- delete this.ssl._secureContext.context;
+ this.ssl._secureContext.context = null;
}
- delete this.ssl;
+ this.ssl = null;
};
TLSSocket.prototype._init = function(socket, wrap) { |
ChALkeR
force-pushed
the
unlink-tlswrap-securecontext
branch
from
0bc9ec8
to
725d7ba
Compare
|
@shigeki No, there is no specific need in |
|
LGTM to me too, landing. |
|
Oh, @ChALkeR ! May I ask you to review this paragraph of the CONTRIBUTING.md: https://github.com/iojs/io.js/blob/master/CONTRIBUTING.md#step-3-commit Particularly, please format the commit log according to it, and please add some contents to it as well ;) Thank you again! |
ChALkeR
force-pushed
the
unlink-tlswrap-securecontext
branch
from
725d7ba
to
f214b9a
Compare
This makes `TLSWrap` and `SecureContext` objects collectable by the incremental gc. `res = null` destroys the cyclic reference in the `reading` property. `this.ssl = null` removes the remaining reference to the `TLSWrap`. `this.ssl._secureContext.context = null` removes the reference to the `SecureContext` object, even though there might be references to `this.ssl._secureContext` somewhere. The `reading` property will now throw an error if accessed after the socket is closed, but that should not happen.
ChALkeR
force-pushed
the
unlink-tlswrap-securecontext
branch
from
f214b9a
to
9ab8f9d
Compare
|
@indutny I added info from the PR to the commit message. Rebased against current master. |
indutny
pushed a commit
that referenced
this pull request
May 4, 2015
This makes `TLSWrap` and `SecureContext` objects collectable by the incremental gc. `res = null` destroys the cyclic reference in the `reading` property. `this.ssl = null` removes the remaining reference to the `TLSWrap`. `this.ssl._secureContext.context = null` removes the reference to the `SecureContext` object, even though there might be references to `this.ssl._secureContext` somewhere. The `reading` property will now throw an error if accessed after the socket is closed, but that should not happen. PR-URL: #1580 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
|
Landed in f7620fb, thank you! |
|
Thanks! |
Fishrock123
pushed a commit
to Fishrock123/node
that referenced
this pull request
May 19, 2015
`StreamBase::AfterWrite` is passing handle as an argument to the `afterWrite` function in net.js. Thus GC should not collect the handle and the request separately and assume that they are tied together. With this commit - request will always outlive the StreamBase instance, helping us survive the GC pass. Same applies to the ShutdownWrap instances, they should never be collected after the StreamBase instance. Fix: nodejs#1580 PR-URL: nodejs#1590 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123
pushed a commit
to Fishrock123/node
that referenced
this pull request
May 19, 2015
This makes `TLSWrap` and `SecureContext` objects collectable by the incremental gc. `res = null` destroys the cyclic reference in the `reading` property. `this.ssl = null` removes the remaining reference to the `TLSWrap`. `this.ssl._secureContext.context = null` removes the reference to the `SecureContext` object, even though there might be references to `this.ssl._secureContext` somewhere. The `reading` property will now throw an error if accessed after the socket is closed, but that should not happen. PR-URL: nodejs#1580 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
ChALkeR
added
the
memory
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This makes
TLSWrapandSecureContextobjects collectable by the incremental gc.res = null;destroys the cyclic reference in thereadingproperty,delete this.sslremoves the remaining reference.delete this.ssl._secureContext.context;makes SecureContext objects collectable by the incremental gc even though there might be references tothis.ssl._secureContextsomewhere.The
readingproperty will throw an error if accessed after the socket is closed. If this is unwanted then the property getter/setter should be altered.Can someone please comment on this part?Should be ok as it is.Test results for 20000 opened-closed non keep-alive connections to localhost (100 parallel requests):