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

v5.4.0 propose #4392

Closed
wants to merge 23 commits into from
Closed

v5.4.0 propose #4392

wants to merge 23 commits into from

Conversation

Fishrock123
Copy link
Contributor

Sorry for the late notice, hoping to still release today. cc @nodejs/ctc / etc

JungMinu and others added 23 commits December 22, 2015 14:28
use `arrow functions` instead of `bind(this)` in order to improve
performance through optimizations.

PR-URL: nodejs#3622
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Update link from github.com/rvagg to github.com/nodejs

PR-URL: nodejs#4331
Reviewed-By: James M Snell <jasnell@gmail.com>
The buffer's write function is documented below the
buf.toString function and all of the docs reference
"buf" instead of "buffer".

PR-URL: nodejs#4324
Reviewed-By: James M Snell <jasnell@gmail.com>
* Use single quotes consistently
* Modernize examples to use template strings and arrow funcs
* Fix a few typos
* Example edits for consistency

PR-URL: nodejs#4282
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This just removes an assignment to `ret` of a value that's not used before
it's overwritten.  Immediately following the assigment is an `if/else` in
which both branches assign to `ret` without using it.

PR-URL: nodejs#4323
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fix a segmentation fault when the debug message handler was called from
a context without an associated `node::Environment`.

Fixes: nodejs#4261
Fixes: nodejs#4322
PR-URL: nodejs#4328
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#4325
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
The initial implementation of setPropByIndex() set the value of an Array
by index during development. Though the final form of the function
simply pushes passed values to an array as passed by arguments. Thus the
functions have been renamed to pushValueToArray() and
push_values_to_array_function() respectively.

Also add define for maximum number of arguments should be used before
hitting the limit of performance increase.

Fixes: 494227b "node: improve GetActiveRequests performance"
PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <fedor@indutny.com>
For performance add headers to the headers Array by pushing them on from
JS. Benchmark added to demonstrate this case.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Improve performance by pushing directory entries to returned array in
batches of 8 using pushValueToArray() in JS. Add benchmarks to
demonstrate this improvement.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Improve performance of process._getActiveHandles by sending handles in
batches to JS to be set on the passed Array. Add test to check proper
active handles are returned.

Alter implementation of GetActiveRequests to match GetActiveHandles'
implementation.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <fedor@indutny.com>
process.hrtime() was performing too many operations in C++ that could be
done faster in JS. Move those operations over by creating a length 4
Uint32Array and perform bitwise operations on the seconds so that it was
unnecessary for the native API to do any object creation or set any
fields.

This has improved performance from ~350 ns/op to ~65 ns/op. Light
benchmark included to demonstrate the performance change.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Set process.env array entries in JS.

PR-URL: nodejs#3780
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Load the certificate chain from the PFX file the same as we do it for a
regular certificate chain.

Fix: nodejs#4127
PR-URL: nodejs#4165
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
On Windows there can exist some race condition where the
notification of the client's `socket.destroy()` isn't received
before the server writes to the socket. This race condition was
more evident/reproducible on a single core system.

This commit fixes the flakiness by waiting until the server's
connection event handler has been called to destroy the client
socket and perform the server socket write.

Fixes: nodejs#4057
PR-URL: nodejs#4342
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: João Reis <reis@janeasystems.com>
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: nodejs#4286
PR-URL: nodejs#4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
String#repeat is quite a bit faster than new Array().join().

PR-URL: nodejs#3900
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
assert.deepEqual: when actual and expected are typed arrays,
wrap them in a new Buffer each to increase performance
significantly.

PR-URL: nodejs#4330
Fixes: nodejs#4294
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
socket.destroy() triggers a 'close' event from the socket which triggers
the onClose handler of HTTPAgent which calls self.removeSocket(). So by
calling self.removeSocket() prior to socket.destroy() we end up with two
calls to self.removeSocket().

If there are pending requests, removeSocket ends up creating a new socket.
So if there are pending requests, each time a request completes, we tear
down one socket and create two more. So the total number of sockets grows
exponentially and without regard for any maxSockets settings. This was
noticed in nodejs#4050. Let's get rid of
the extra calls to removeSocket so we only call it once per completed
request.

PR-URL: nodejs#4172
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
These types are no longer used in the file and V8 4.9 no longer defines these
types anymore.

PR-URL: nodejs#4381
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
If the deprecated NODE_REPL_HISTORY_FILE is set to default
node history file path ($HOME/.node_repl_history) and the file
doesn't exist, then node creates the file and then crashes when
it tries to parse that file as JSON thinking that it's an older
JSON formatted history file. This fixes that bug.

This patch also prevents node repl from throwing if the old
history file is empty or if $HOME/.node_repl_history is empty.

Fixes: nodejs#4102
PR-URL: nodejs#4108
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Notable changes:

* general: Several minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - **node**: Improved performance of hrtime() (Trevor Norris)
nodejs#3780
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
* module: Errors during require() now provide more information (Brian
White) nodejs#4287
@Fishrock123 Fishrock123 added the meta Issues and PRs related to the general management of the project. label Dec 22, 2015
@MylesBorins
Copy link
Contributor

redacted... I am way off

@mscdex
Copy link
Contributor

mscdex commented Dec 22, 2015

FWIW I wasn't sure on the semver-ness of 72b6b4f. What was the decision on changes to error messages (the error stack trace text in this case)?

@MylesBorins
Copy link
Contributor

@jasnell ^^^

@MylesBorins
Copy link
Contributor

CITGM Passed

Passing Modules

  • lodash
    • version 3.10.1
  • underscore
    • version 1.8.3
  • request
    • version 2.67.0
  • commander
    • version 2.9.0
  • q
    • version 1.4.1
  • coffee-script
    • version 1.10.0
  • through2
    • version 2.0.0
  • glob
    • version 6.0.1
  • gulp-util
    • version 3.0.7
  • jade
    • version 1.11.0
  • socket.io
    • version 1.3.7
  • fs-extra
    • version 0.26.3
  • body-parser
    • version 1.14.2
  • uglify-js
    • version 2.6.1
  • rimraf
    • version 2.4.5
  • david
    • version 7.0.0
  • eslint
    • version 1.10.3
  • tape
    • version 4.2.2
  • browserify
    • version 12.0.1
  • watchify
    • version 3.6.1
  • stylus
    • version 0.53.0
  • level
    • version 1.4.0
  • torrent-stream
    • version 1.0.0
  • gulp
    • version 3.9.0
  • moment
    • version 2.10.6

Flakey Modules

  • express
    • version 4.13.3
    • Error: The canary is dead.
  • jquery
    • version 2.1.4
    • Error: The canary is dead.
  • react
    • version 0.14.3
    • Error: Install Failed

@MylesBorins
Copy link
Contributor

Looks like moment is expect a global install of the grunt-cli, which was passing on my system due to a global grunt. After installing globally the tests pass

edited above commit to reflect update

@Fishrock123
Copy link
Contributor Author

hmmm, to do this tomorrow or not? Or still get it out today? Idk.

@ofrobots
Copy link
Contributor

@Fishrock123 Are you filtering commits from issues labelled dont-land-on-v5.x. 2888ec3 is one such commit. There is no pressing need to backport it, although is no harm either I guess, as long as it compiles.

@Fishrock123
Copy link
Contributor Author

Are you filtering commits from issues labelled dont-land-on-v5.x.

No? I was unaware that existed, and our tooling does not cover it. I don't remember @nodejs/release being notified about it.

@ofrobots
Copy link
Contributor

See @rvagg comment here: #4160 (comment)

@rvagg
Copy link
Member

rvagg commented Dec 23, 2015

Notification to @nodejs/release about it, including suggested branch-diff commandline, here: #4181 (comment)

But you're forgiven for not noticing, there's just so much going on!

@rvagg
Copy link
Member

rvagg commented Dec 23, 2015

FWIW I'd be fine with punting on this for a week to save people a pre-Christmas batching with the v4 release coming out, the commits list is neither huge or compelling. Your call though @Fishrock123.

@rvagg
Copy link
Member

rvagg commented Jan 4, 2016

@Fishrock123 can you handle a Tuesday release this week? If not, let me know and I can take over.

@Fishrock123
Copy link
Contributor Author

Should be able to.

@Fishrock123 Fishrock123 changed the title v5.3.1 propose v5.4.0 propose Jan 5, 2016
@Fishrock123
Copy link
Contributor Author

Working on this again. At first glance it looks like it will be v5.4.0 due to:

  • [1c5d4f2453] - (SEMVER-MINOR) http: 451 status code "Unavailable For Legal Reasons" (Max Barinov) #4377

@NawarA
Copy link

NawarA commented Jan 6, 2016

Anyway we can include V8 4.7.80.25 in the v5.4 proposal? It was committed to Master on Dec 2, 2015. Or are we holding off until Node v6.0?

@Fishrock123
Copy link
Contributor Author

@NawarA No, sorry, that would be a breaking change.

@Fishrock123 Fishrock123 mentioned this pull request Jan 6, 2016
@Fishrock123
Copy link
Contributor Author

Moving to #4547

@Fishrock123 Fishrock123 closed this Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.