Skip to content
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

tls_wrap: proxy handle methods in prototype #1108

Closed
wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Mar 9, 2015

Set proxied methods wrappers in TLSWrap prototype instead of doing it
on every socket allocation. Should speed up things a bit and will
certainly make heapsnapshot less verbose.

cc @iojs/crypto

Set proxied methods wrappers in `TLSWrap` prototype instead of doing it
on every socket allocation. Should speed up things a bit and will
certainly make heapsnapshot less verbose.
@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

cc @iojs/collaborators too

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

CI: good

if (this._parent[name])
return this._parent[name].apply(this._parent, arguments);
};
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we care about the extra performance lost to creating a closure and referring to the call as this._parent[name] instead of this._parent.<name>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it shouldn't be critical here.

@trevnorris
Copy link
Contributor

The change looks good. Just have a question.

@bnoordhuis
Copy link
Member

I see there is one TLS test failing on Windows but I'm not sure if it's related to this change...

@rvagg
Copy link
Member

rvagg commented Mar 9, 2015

3 failures on windows are persistent and I don't believe they are related to this

@bnoordhuis
Copy link
Member

I checked previous runs and parallel/test-tls-over-http-tunnel seems to be failing consistently (and it's always the same 3 or 4 tests that fail.) LGTM then.

indutny added a commit that referenced this pull request Mar 9, 2015
Set proxied methods wrappers in `TLSWrap` prototype instead of doing it
on every socket allocation. Should speed up things a bit and will
certainly make heapsnapshot less verbose.

PR-URL: #1108
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@indutny
Copy link
Member Author

indutny commented Mar 9, 2015

Landed in 8431fc5, thank you everyone!

@indutny indutny closed this Mar 9, 2015
@indutny indutny deleted the feature/better-tls-wrap branch March 9, 2015 18:51
@rvagg rvagg mentioned this pull request Mar 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants