Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

No closures #23

Closed
wants to merge 3 commits into from
Closed

No closures #23

wants to merge 3 commits into from

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Dec 5, 2016

includes #21.

I have used bind in some cases where perf was not needed and we needed to pass some parameters to the function. bind is as fast as creating a closure in the latest V8, but it is not hoisted and so we avoid it altogether for the happy-path.

jasnell

This comment was marked as off-topic.

@jasnell
Copy link
Member

jasnell commented Dec 5, 2016

Want to close #21 since this includes it?

evanlucas

This comment was marked as off-topic.

@mcollina
Copy link
Member Author

mcollina commented Dec 5, 2016

@jasnell yes.

@evanlucas adresses.

evanlucas

This comment was marked as off-topic.

@mcollina
Copy link
Member Author

mcollina commented Dec 6, 2016

@evanlucas @jasnell I moved away from Map() and converted it back to {}. It should solve most of the issues. Unfortunately for-of loops are really slow :(.

@evanlucas
Copy link
Contributor

@mcollina won't moving back to an object prevent setting headers like __proto__?

@mcollina
Copy link
Member Author

mcollina commented Dec 6, 2016

@evanlucas this is the behavior that we have on require('http'), and it is faster than Map for iterating over all the pairs in the map.

https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L365

There is PR to change that, see nodejs/node#6102.

@targos
Copy link
Member

targos commented Dec 6, 2016

I suppose http2 support won't land before v8.0.0? By that time, we will have a much more recent version of V8 in which the for-of loop could be fast enough.
I think we should'nt make those engine-related micro optimizations too early.

@mightyiam
Copy link
Contributor

I like what @targos is saying. Optimise as required, when required.

@mcollina
Copy link
Member Author

mcollina commented Dec 6, 2016

I strongly disagree. There are both perf and compatibility issue with http1 -> see the discussion there.
As far as I understand, there is no magical "for of" optimization landing anytime soon in V8.

This particular code makes no sense if it is not as fast as possible.

@jasnell
Copy link
Member

jasnell commented Dec 6, 2016

@mcollina ... switching away from Map is fine but I think using a prototype-less object (Object.create(null)) would be better. We wanted to make such a change in the http/1 stuff but haven't been able to due to backwards compatibility issues that we simply do not have in this case. Using the prototype-less object allows us to not have to deal with issues like __proto__

@jasnell
Copy link
Member

jasnell commented Dec 6, 2016

@targos: the goal is to get this into 8.x as close to 8.0.0 as possible as experimental.

@mcollina
Copy link
Member Author

mcollina commented Dec 6, 2016

@jasnell should be good now. Please review.

jasnell

This comment was marked as off-topic.

@jasnell
Copy link
Member

jasnell commented Dec 7, 2016

@mcollina ... I'm going to get this landed so I can start building off of it.

@jasnell
Copy link
Member

jasnell commented Dec 7, 2016

Landed!

@jasnell jasnell closed this Dec 7, 2016
@jasnell jasnell mentioned this pull request Dec 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants