64bit support for dtrace ustack helper #4507
Conversation
| #define NODE_OFF_EXTSTR_DATA 0x4 | ||
| #else | ||
| #define NODE_OFF_EXTSTR_DATA 0x8 | ||
| #endif |
There was a problem hiding this comment.
#define NODE_OFF_EXTSTR_DATA sizeof(void*)?
|
Thanks again for doing these changes! I'm trying to figure out what's actually changed, but there's a lot of spurious delta here, mostly related to style. The style changes seem unnecessary, since D != C (though D also uses cpp), and I assume Node doesn't have specific style guidelines for D. The comment on line 20 could be clearer. AFAICT, the way the helper identifies and decodes small integers is the same for both 32-bit and 64-bit. What do you mean "things are a bit different"? The most important thing is the test plan. How has this change been tested? |
|
@davepacheco You can filter out the whitespace changes with ?w=1: https://github.com/joyent/node/pull/4507/files?w=1 EDIT: But I agree that the whitespace changes are unnecessary and distracting. |
|
@davepacheco we're sharing the same style between all C-like languages used in node. Therefore, we're trying to avoid using tabs where it's not needed and etc. About that comment, I agree that it's misleading, I think I did it after spending few hours before figuring it out. And later, forgot about it :) Considering that this test will only work on smartos platform, do you guys have any ideas on how it could be tested? |
|
Thanks, @bnoordhuis. That helps a lot, but the vast majority of the delta is still not semantically relevant. @indutny: re: style: The original file was written in the standard D style. People coming to this file with experience in D will expect 8-column tabs, for example. See all files in /usr/lib/dtrace on both OS X and SmartOS, the DTrace Toolkit, and most other D files out there. Re: testing: I'd suggest covering at least these cases:
If possible, I'd also try getting flame graphs for a variety of Node programs to cover other cases we haven't thought of. |
|
@davepacheco style is really a silly question. My opinion, is that people just use what it seems to be right in their projects, linux kernel is using different C-style too, so sticking to some specific style is just a matter of taste. Needless to say I respect your experience, and if you think this really matters here - I can roll this changes out. |
|
And yeah, sorry for mixing it into one commit... I did it early, and it's hard to separate this stuff now. |
|
@davepacheco updated comment. |
|
@indutny Thanks for the comment update. Agreed on style being somewhat arbitrary; that's why my strong preference is to avoid restyling it as part of this otherwise small (by LOC) change. This is looking good! The testing is the biggest open issue. There are a bunch of corner cases I mentioned above, and since we use this heavily, I want to be sure we don't break 32-bit at all and I hope we can catch any new 64-bit-specific issues before people start playing with it. Thanks again for doing this! |
* buffer:
- You can now supply an encoding argument when filling a
Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
an existing Buffer will also work with
Buffer#fill(buffer[, start[, end]]). See the API documentation for
details on how this works. (Trevor Norris) nodejs#4935
- Buffer#indexOf() no longer requires a byteOffset argument if you
also wish to specify an encoding:
Buffer#indexOf(val[, byteOffset][, encoding]).
(Trevor Norris) nodejs#4803
* child_process: spawn() and spawnSync() now support a 'shell' option
to allow for optional execution of the given command inside a shell.
If set to true, cmd.exe will be used on Windows and /bin/sh
elsewhere. A path to a custom shell can also be passed to override
these defaults. On Windows, this option allows .bat. and .cmd files
to be executed with spawn() and spawnSync(). (Colin Ihrig) nodejs#4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
strict limitation of allowable header characters.
(James M Snell) nodejs#5237
* dgram: socket.send() now supports accepts an array of Buffers or
Strings as the first argument. See the API docs for details on how
this works. (Matteo Collina) nodejs#4374
* http: Fix a bug where handling headers will mistakenly trigger an
'upgrade' event where the server is just advertising its protocols.
This bug can prevent HTTP clients from communicating with HTTP/2
enabled servers. (Fedor Indutny) nodejs#4337
* net: Added a listening Boolean property to net and http servers to
indicate whether the server is listening for connections.
(José Moreira) nodejs#4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
it from inside another MakeCallback() call no longer causes the
nextTick queue or Promises microtask queue to be processed out of
order. (Trevor Norris) nodejs#4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
TLS protocol version of the current connection. (Brian White) nodejs#4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
new vm.Script() to interact with V8's code cache. When a new
vm.Script object is created with the 'produceCachedData' set to true
a Buffer with V8's code cache data will be produced and stored in
cachedData property of the returned object. This data in turn may be
supplied back to another vm.Script() object with a 'cachedData'
option if the supplied source is the same. Successfully executing a
script from cached data can speed up instantiation time. See the API
docs for details. (Fedor Indutny) nodejs#4777
* performance: Improvements in:
- process.nextTick() (Ruben Bridgewater) nodejs#5092
- path module (Brian White) nodejs#5123
- querystring module (Brian White) nodejs#5012
- streams module when processing small chunks (Matteo Collina) nodejs#4354
PR-URL: nodejs/node#5295
n/t
/cc @davepacheco @bnoordhuis