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

re-add WriteReq and CorkedRequest constructors #255

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Jan 3, 2017

This PR is in anticipation of nodejs/node#10558 (do not land this before the node core PR).

Also, this should land before #253 so that the replacements are done in the correct order.

/cc @mcollina

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@calvinmetcalf
Copy link
Contributor

so once nodejs/node#10558 lands I'll try merging in this and #253

@calvinmetcalf
Copy link
Contributor

so I've got a couple questions

  1. is this that the perf benefits of object literals vs constructors have changed in more recent versions of v8 or is it that better perfing has shown that object literals were better all along
  2. why is this beneficial in node core but NOT in readable stream?

have been out of the loop the last week or 2 so sorry if this has been answered already

@mcollina
Copy link
Member

mcollina commented Jan 4, 2017

is this that the perf benefits of object literals vs constructors have changed in more recent versions of v8 or is it that better perfing has shown that object literals were better all along

Only most recent versions

why is this beneficial in node core but NOT in readable stream?

It will cause a slowdown for older node versions.

@mscdex mscdex mentioned this pull request Jan 4, 2017
@mcollina mcollina mentioned this pull request Mar 11, 2017
@mcollina
Copy link
Member

Part of #262

@mcollina mcollina closed this Mar 14, 2017
@mcollina mcollina reopened this Mar 14, 2017
@mcollina
Copy link
Member

Merging this have been a mistake. This should be merged when Node.js hits v8.0.0, as the the HTTP changes lands.

@calvinmetcalf
Copy link
Contributor

@mcollina there is a merge conflict here, can you rebase it ?

@mcollina
Copy link
Member

we need this for #290. I'll see what I can do but I am traveling this week.

@calvinmetcalf
Copy link
Contributor

ok I can probably do it too, I'm not 100% on it though like how many libraries are using corked writable streams in the hot path and in older node using readable-streams. Wouldn't they also be having similar if not bigger performance issues from using that older node and they'd get bigger benefits from upgrading ?

@mcollina mcollina mentioned this pull request Jun 14, 2017
@mcollina
Copy link
Member

Closing as it is now part of #290.

@mcollina mcollina closed this Jun 14, 2017
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.

3 participants