build,src: add Intel Vtune profiling support #4135
Conversation
Dependency upgrades. PR-URL: nodejs#3686 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: James M Snell <[email protected]>
This patch - issues a TAP plugin parsable message on non darwin/windows boxes - uses `const` wherever applicable - moves the test to parallel PR-URL: nodejs#2599 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fix configure_library() to produce correct LDFLAGS when configuring with
prebuilt 3rd-party libraries (libuv, openssl, etc) using `pkg-config' or
`--shared-{LIBRARY}-includes=xxx --shared-{LIBRARY}-libpath=xxx'.
PR-URL: nodejs#3135
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Johan Bergström <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
PR-URL: nodejs#2796 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]>
Update the documentation for `process.stdout` and `process.stdout` to clarify that writes can block when stdio is redirected to a file. In all other cases, it's non-blocking. PR-URL: nodejs#3170 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Added referenced method links. PR-URL: nodejs#3187 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
When the user hits `^C` in the REPL show more info about `.exit`. The idea was to give more info to the user when they hit ^C. Current version just displays `(^C again to quit)` and most of the users are not aware of the `.exit` command that would Exit the repl. PR-URL: nodejs#3368 Reviewed-By: James M Snell <[email protected]>
Add description of user responsibility in the choice of cypto algorithms and its key length. Some of recommendations for the safer use are also described. PR-URL: nodejs#3479 Reviewed-By: James M Snell <[email protected]>
This reverts 8cee8f5 which was causing stdin to behave strangely on Windows 8 and 10. The suspected explanation for the issue is that there might be a race condition occuring when stdin._readableState.reading is set indirectly through `push('')`. PR-URL: nodejs#3490 Fixes: nodejs#2996 Fixes: nodejs#2504 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
When setTimeout() and setInterval() are called with `delay` greater than TIMEOUT_MAX (2147483647), the supplied value is ignored and 1 is used instead. Add a note about this in the timers docs. PR-URL: nodejs#3512 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#3533 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Bug spotted by @bnoordhuis while doing code review on nodejs#3534 Refs: nodejs#3534 (comment) PR-URL: nodejs#3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
When stream.flush() is called without a callback, an empty listener is being added. Since flush may be called multiple times to push SSE's down to the client, multiple noop listeners are being added. This in turn causes the memory leak detected message. PR-URL: nodejs#3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
This test assures that if flush is called while the zlib object needs to be drained that it will defer the callback until after the drain. PR-URL: nodejs#3534 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>
as of https://github.com/nodejs/node/blob/v5.x/src/node_buffer.cc#L555 buf.copy returns the number of bytes copied. PR-URL: nodejs#3555 Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#3565 Reviewed-By: Brian White <[email protected]> Reviewed-By: James M Snell <[email protected]>
This makes the code spans in the API docs more visible and therefore readable by adding some background color. PR-URL: nodejs#3573 Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
`debuglog` uses `%j` as a placeholder for replacement with `JSON.stringify`. So that `JSON.stringify` is only called when the appropriate debug flag is on. The other `%s` changes are for style consistency. PR-URL: nodejs#3578 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
The examples for implementing the simplified constructor API was missing some details on its correct usages. PR-URL: nodejs#3602 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chris Dickinson <[email protected]>
Use path join to construct the path instead of concatenating strings. Replace backslash with double backslash so that they are escaped correctly in the string passed to REPL. PR-URL: nodejs#3608 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fix regarding description of the following functions: Certificate.exportPublicKey(spkac) Certificate.exportChallenge(spkac) The descriptions were applied incorrectly. PR-URL: nodejs#3614 Reviewed-By: Ben Noordhuis <[email protected]>
Previously, if we are unable to open the history file, an error would be thrown. Now, print an error message that we could not open the history file, but don't fail. Fixes: nodejs#3610 PR-URL: nodejs#3630 Reviewed-By: Jeremiah Senkpiel <[email protected]>
As per nodejs#2525 a bunch of WGs are renamed from iojs-* to nodejs-*. Update the WORKING_GROUPS.md to match. Note specifically iojs-cn and iojs-tw were renamed to nodejs-zh-CN and nodejs-zh-TW respectively. Fixes: nodejs#3247 PR-URL: nodejs#3634 Reviewed-By: James M Snell <[email protected]>
* A known issue was resolved but not removed from the list * The wrong date was documented in the changelog for v4.2.2 PR-URL: nodejs#3650 Reviewed-By: Michaël Zasso <[email protected]>
Fixed an intermittent issue on AIX where the 600ms timeout was reached before the 'connection' event was fired. This resulted in a failure as serverConnection would be undefined and the assert.equal would throw an error. Changed the flow of the test so that the timeout is only set after a connection has been made. PR-URL: nodejs#3646 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
Minor typo fix in the list of keys Reviewed-By: James M Snell <[email protected]> PR-URL: nodejs#3649
When a compiled library file does not have the proper format, musl returns the error message ENOEXEC as 'Exec format error' but glibc returns 'file too short' if the file is under a certain size. Reference: http://git.musl-libc.org/cgit/musl/tree/src/errno/__strerror.h#n46 This patch consists of tolerating musl's error. PR-URL: nodejs#3657 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Johan Bergström <[email protected]>
There is currently no information in the Collaborators guide regarding LTS. This commit adds some basic copy explaining what LTS is, what is considered for LTS, and a simple way collaborators can help. Reviewed-By: James M Snell <[email protected]> Reviewed-By: Steven R. Loomis <[email protected]> PR-URL: nodejs#3442
PR-URL: nodejs#3668 Reviewed-By: Julien Gilli <[email protected]>
test fails intermittently due to the assertion that the 'disconnect' event should come before the 'exit' event. This is caused be the non-deteministic behaviour of pollset_poll[1] on AIX (see deps/uv/src/unix/aix.c). This API makes no garauntee for the order in which file descriptors are returned. On linux epoll_wait[2] is used, which also does not make a garauntee on order of file descriptors returned. In the failing case we recieve our file descriptor with a callback of uv__signal_event (which causes JavaScript to receive the exit event) before our file descriptor with uv__stream_io as its callback (which in turn causes JavaScript receive the disconnect event). This change simply removes the assertion that the disconnect event happens before exit event and processes the test regardless of which event comes first. [1] https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.ai x.basetrf1/pollset.htm [2] http://linux.die.net/man/2/epoll_pwait PR-URL: nodejs#3666 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
See nodejs#3957 for details and examples failures. Ref: nodejs#3957 Reviewed-By: Ben Noordhuis <[email protected]> PR-URL: nodejs#4006
Mark test-net-socket-local-address flaky on FreeBSD. It times out with some frequency on CI. Ref: nodejs#2475 PR-URL: nodejs#4016 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
This feature supports the Intel Vtune profiling support for JITted JavaScript on IA32 / X64 / X32 platform. The advantage of this profiling is that the user / developer of NodeJS application can get the detailed profiling information for every line of the JavaScript source code. This information will be very useful for the owner to optimize their applications. This feature is a compile-time option. For windows platform, the user needs to pass the following parameter to vcbuild.bat: "enable-vtune" For other OS, the user needs to pass the following parameter to ./configure command: "--enable-vtune-profiling" cherry-pick: nodejs#3785 Reviewed-By: Ben Noordhuis <[email protected]> Conflicts: configure
|
@thealphanerd @rvagg @jasnell @bnoordhuis |
Fishrock123
added
build
|
This appears to have been labeled semver-minor in the original PR? How are we going to deal with that for v4.x? cc @nodejs/lts |
|
Oy, yeah, if this is semver-minor it should not land in v4.x, I had missed that entirely in my read through on the initial PR |
|
Based on my reading, this feature is only enabled a special build with a compile time flag. I wouldn't consider this semver-minor. Even if it this feature was not behind a compile-time flag, I think it is overly pedantic to consider tooling improvements like this to be semver minor. IMO, this waters down the value of an LTS. Why don't we want LTS users to be able to profile their applications? Specially when we are confident that the tooling changes have no impact on stability. Instead of a pedantic 'LTS means no semver-minor', lets use this criteria:
See also #3609. |
New flags are, the program will error if you roll back to an older version with them. (As discussed for #3609)
I don't really disagree about some of this. We should probably just learn from all of it and allow occasional (say maybe quarterly at minimum) |
These are opt-in flags. The users who don't opt-in can roll-back anytime. The users who opt-into using the new flag, don't have an expectation that they can continue using a new feature when they roll-back. In this particular case, they have to do a custom build of node. |
|
@ofrobots ... in general these should be ok, but the default position for LTS should be to say no and only allow semver-minor if there's a good enough reason to let it in. I think @Fishrock123's suggestion of doing a quarterly review makes sense, particularly if we give LTS users a heads up in advance that such changes may be coming and clearly explain the impact of their use. |
|
Oh... and the PR definitely needs to be cleaned up, lots of extraneous commits in there... |
|
@cdai2 have you had a chance to clean this up so that there are not so many commits against v4.x-staging? |
jasnell
force-pushed
the
v4.x-staging
branch
from
08be311
to
bd8b2f0
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
from
592237f
to
cf7ebd3
Compare
MylesBorins
force-pushed
the
v4.x-staging
branch
2 times, most recently
from
8b08780
to
524b083
Compare
|
@cdai2 it looks like this commit has fallen behind the staging branch quite a bit. Would you be able to rebase + fix any conflicts? We are working on a 4.4 release candidate right now so this is the perfect opportunity to get this patch in |
|
@nodejs/lts if anyone wants to take this on / champion for this change please feel free to help move it forward. There is a nice window of about 2 weeks where we can potentially get this in the lts release |
|
@nodejs/lts LAST CALL. If someone does not get this up and running today or tomorrow we are not going to likely get this in v4.4.0 and I am unsure of a timeline for another minor on v4 (if there will be one). /cc @cdai2 |
|
Hi, TheAlphaNerd. |
|
Continues in #5527. |
This feature supports the Intel Vtune profiling support for JITted
JavaScript on IA32 / X64 / X32 platform. The advantage of this profiling
is that the user / developer of NodeJS application can get the detailed
profiling information for every line of the JavaScript source code.
This information will be very useful for the owner to optimize their
applications.
This feature is a compile-time option. For windows platform, the user
needs to pass the following parameter to vcbuild.bat: "enable-vtune"
For other OS, the user needs to pass the following parameter to
./configure command: "--enable-vtune-profiling"
cherry-pick: #3785
Reviewed-By: Ben Noordhuis [email protected]