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.

Sign up for GitHub

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

Jump to bottom

[v8.x backport] benchmarks: improve code base #21169

Closed
wants to merge 59 commits into from
Closed

[v8.x backport] benchmarks: improve code base #21169

wants to merge 59 commits into from

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented Jun 6, 2018 •
edited
Loading

This PR backports #18320 to v8.x-staging.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @BridgeAR @MylesBorins

Peter Marton and others added 30 commits May 15, 2018 17:26
@MylesBorins
http: add options to http.createServer()
4994b7f 
This adds the optional options argument to `http.createServer()`.
It contains two options: the `IncomingMessage` and `ServerReponse`
option.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@hekike @MylesBorins
http2: add http fallback options to .createServer
5bfc1d1 
This adds the Http1IncomingMessage and Http1ServerReponse options
to http2.createServer().

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@MylesBorins
http2: add req and res options to server creation
852b996 
Add optional Http2ServerRequest and Http2ServerResponse options
to createServer and createSecureServer. Allows custom req & res
classes that extend the default ones to be used without
overriding the prototype.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15560
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@vsemozhetbyt @MylesBorins
doc: fix typo in http2.md
0829b54 
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18872
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@danbev @MylesBorins
http2: pass session to DEBUG_HTTP2SESSION2
f7d96ac 
When configure with --debug-http2 --debug-nghttp2 the following
compilation error is generated:

DEBUG_HTTP2SESSION2(this, "fatal error receiving data: %d", ret);
                          ^
../src/node_http2.cc:1690:27:
error: invalid use of 'this' outside of a non-static member function

1 errors generated.

OnStreamReadImpl is static and I think the intention was to pass in the
session variable here.

PR-URL: nodejs#20815
Refs: nodejs#20806
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@refack @MylesBorins
tools: speed up lint-md-build
83cd22e 
by using package-lock.json

PR-URL: nodejs#16945
Fixes: nodejs#16628
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@addaleax @MylesBorins
src: remove superfluous check in backtrace_posix.cc
bdbef5f 
The error check doesn't matter because a failure would be ignored
as part of the loop condition.

PR-URL: nodejs#16950
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@AndreasMadsen @MylesBorins
trace_events: add file pattern cli option
141e3e9 
Allow the user to specify the filepath for the trace_events log file
using a template string.

Backport-PR-URL: nodejs#19145
PR-URL: nodejs#18480
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins
src, test: node internals' postmortem metadata
b2b85e5 
Before these changes, only V8 added postmortem metadata to Node's
binary, limiting the possibilities for debugger's developers to add some
features that rely on investigating Node's internal structures.

These changes are first steps towards empowering debug tools to
navigate Node's internal structures. One example of what can be
achieved with this is shown at nodejs/llnode#122 (a command which prints
information about handles and requests on the queue for a core dump
file). Node postmortem metadata are prefixed with nodedbg_.

This also adds tests to validate if all postmortem metadata are
calculated correctly, plus some documentation on what is postmortem
metadata and a few care to be taken to avoid breaking it.

Ref: nodejs/llnode#122
Ref: nodejs/post-mortem#46

Backport-PR-URL: nodejs#19176
PR-URL: nodejs#14901
Refs: nodejs/post-mortem#46
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@bnoordhuis @MylesBorins
test: fix cctest -Wunused-variable warning
d27ca6d 
Backport-PR-URL: nodejs#19176
PR-URL: nodejs#18530
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@joyeecheung @MylesBorins
src: do not redefine private for GenDebugSymbols
591812f 
Redefining private breaks any private inheritance in the
included files. We can simply declare GenDebugSymbols()
as friends in related classes to gain the access that we need.

Backport-PR-URL: nodejs#19176
PR-URL: nodejs#18653
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@joyeecheung @MylesBorins
src: expose uv.errmap to binding
3a7ad60 
Add a errno -> [error code, uv error message] map to the uv binding
so the error message can be assembled in the JS layer.

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#17338
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung @MylesBorins
util: implement util.getSystemErrorName()
4a55524 
Reimplement uv.errname() as internal/util.getSystemErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18186
Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@joyeecheung @MylesBorins
errors: lazy load util in internal/errors.js
05076ac 
Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18358
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
@joyeecheung @MylesBorins
util: skip type checks in internal getSystemErrorName
de86203 
Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18546
Reviewed-By: James M Snell <[email protected]>
@joyeecheung @MylesBorins
errors: move error creation helpers to errors.js
64f4b19 
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.

- Move util._errnoException and util._exceptionWithHostPort
  into internal/errors.js and simplify their logic so it's
  clearer what the properties these helpers create.
- Move the errnoException helper in dns.js to internal/errors.js
  into internal/errors.js and rename it to dnsException. Simplify
  it's logic so it no longer calls errnoException and skips
  the unnecessary argument checks.

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18546
Reviewed-By: James M Snell <[email protected]>
@BridgeAR @MylesBorins
errors: make message non-enumerable
a974479 
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#19719
Fixes: nodejs#19716
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@danbev @MylesBorins
test: set clientOpts.port property
4406f6e 
Currently this test will overwrite the clientOpts object with the port,
instead of setting the port property on the clientOpts object which
looks like the original intent.

Doing this the test fails reporting that the fake-cnnic-root-cert has
expired. This is indeed true:
$ openssl x509 -in test/fixtures/keys/fake-cnnic-root-cert.pem \
-text -noout
Certificate:
        ...
        Validity
            Not Before: Jun  9 17:15:16 2015 GMT
            Not After : Mar 29 17:15:16 2018 GMT

This commit sets the errorCode to CERT_HAS_EXPIRED. I tried updating the
certificate using test/fixtures/keys/Makefile but then no error is
thrown and I'm currently looking into this.

Backport-PR-URL: nodejs#20776
PR-URL: nodejs#19767
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danbev @MylesBorins
test: remove test case 0 from tls-cnnic-whitelist
4f05d1b 
I looks like this test has not worked as expected since commit
2bc7841 ("test: use random ports
where possible"). The test in that commit checked for `CERT_REVOKED`
which was returned by CheckWhitelistedServerCert.

CheckWhitelistedServerCert was later removed in commit
6ee4228 ("src: drop CNNIC+StartCom
certificate whitelisting").

I'm suggesting that this test case be removed as I don't think it is
valid anymore.

Backport-PR-URL: nodejs#20776
PR-URL: nodejs#19767
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@danbev @MylesBorins
crypto: remove BIO_set_shutdown
6251f2a 
I've not been able to find any reason for calling
BIO_set_shutdown(bio, 1). This is done by default for the following
versions of OpenSSL:

https://github.com/openssl/openssl/blob/OpenSSL_1_1_0/
crypto/bio/bio_lib.c#L26

https://github.com/openssl/openssl/blob/OpenSSL_1_0_1/
crypto/bio/bio_lib.c#L90

https://github.com/openssl/openssl/blob/OpenSSL_1_0_2/
crypto/bio/bio_lib.c#L88

https://github.com/openssl/openssl/blob/OpenSSL_1_0_0/
crypto/bio/bio_lib.c#L90

This commit removes the call and the comment.

PR-URL: nodejs#17542
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@refack @MylesBorins
tools: don't lint-md as part of main lint target
4512116 
PR-URL: nodejs#17587
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax @MylesBorins
tls: use correct class name in deprecation message
a6d6325 
`tls.Socket` does not exist, and the deprecation message
should refer to `tls.TLSSocket` (like the documentation
for the deprecation message already does).

PR-URL: nodejs#17561
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@TimothyGu @MylesBorins
src: make FSEventWrap/StatWatcher::Start more robust
4cd0ab5 
PR-URL: nodejs#17432
Fixes: nodejs#17430
Reviewed-By: James M Snell <[email protected]>
@prohaejin @MylesBorins
url: added space to class string of iterator objects
e0a780b 
PR-URL: nodejs#17558
Fixes: nodejs#17540
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Kimeiga @MylesBorins
url: added url fragment lookup table
d81a3f4 
Percent-encoded additional characters in fragment state with new
FRAGMENT_ENCODE_SET lookup table. The fragment percent-encode set
includes the C0 control percent-encode set and code points U+0020,
U+0022, U+003C, U+003E, and U+0060.

PR-URL: nodejs#17627
Fixes: nodejs#17540
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Daijiro Wachi <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig @MylesBorins
test: add test for postmortem metadata validation
b07b9f2 
This commit adds a test to validate postmortem debugging metadata.
When this test runs, it can check for the presence of metadata
constants used by tools such as llnode and mdb and report if any
have accidentally been removed.

PR-URL: nodejs#17685
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
@maclover7 @MylesBorins
lib, src: use process.config instead of regex
524efb9 
Is safer to use a `process.binding(config)` defined boolean, than to
regex on `process.execArgv`. Also, this better falls in line with the
conventions of checking flags passed to the executable.

PR-URL: nodejs#17814
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott @MylesBorins
test: fix unreliable async-hooks/test-signalwrap
3aefc25 
Use an interval to keep the event loop open so the test does not exit
before receiving all signals fom asynchronous `exec()` calls.

PR-URL: nodejs#17827
Fixes: nodejs#14070
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@tniessen @MylesBorins
src: remove duplicate words in comments
14f29e3 
PR-URL: nodejs#17939
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@tniessen @MylesBorins
http2: remove duplicate words in comments
b046412 
PR-URL: nodejs#17939
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BridgeAR added 3 commits June 6, 2018 15:10
@BridgeAR @ryzokuken
benchmark: (http(2)) refactor
8a97757 
PR-URL: nodejs#18320
Reviewed-By: James M Snell <[email protected]>
@BridgeAR @ryzokuken
benchmark: (timers) refactor
b186741 
PR-URL: nodejs#18320
Reviewed-By: James M Snell <[email protected]>
@BridgeAR @ryzokuken
benchmark: refactor
ab4365e 
PR-URL: nodejs#18320
Reviewed-By: James M Snell <[email protected]>
@ryzokuken
Copy link
Contributor Author

Wait, looks like some conflicts were botched up by VSCode, will fix them.

@ryzokuken
Copy link
Contributor Author

Nothing wrong with VSCode, forgot to save these files 😅

PTAL now and tell me how it looks.

@ryzokuken
Copy link
Contributor Author

ping @BridgeAR @MylesBorins

@MylesBorins
Copy link
Contributor

@nodejs/benchmarking can someone familliar with the benchmarking suite test that this all works?

@codebytere codebytere force-pushed the v8.x-staging branch 2 times, most recently from 6e5e692 to c2ae01f Compare August 3, 2018 00:30
@MylesBorins MylesBorins force-pushed the v8.x-staging branch 3 times, most recently from 50ccfec to 0c9760d Compare August 9, 2018 23:28
@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

This needs to be updated and checked again @nodejs/lts

@jasnell jasnell added stalled Issues and PRs that are stalled. v8.x labels Sep 10, 2018
@BethGriggs
Copy link
Member

@ryzokuken could you rebase this please?

@ryzokuken
Copy link
Contributor Author

Sorry, @BethGriggs. I'll try to do that ASAP.

@MylesBorins
Copy link
Contributor

ping re: rebase

@ryzokuken
Copy link
Contributor Author

@MylesBorins @BethGriggs I have been trying to do that, but there's way too many conflicts ATM, especially due to dependency bumps. Mind if I make a fresh one instead? I have a feeling that'll be easier.

@MylesBorins
Copy link
Contributor

MylesBorins commented Nov 4, 2018 via email

@ryzokuken
Copy link
Contributor Author

@MylesBorins awesome! Thanks. Will do.

@MylesBorins
Copy link
Contributor

@ryzokuken are you still going to work on this or should we close the PR?

@ryzokuken
Copy link
Contributor Author

ryzokuken commented Dec 26, 2018 •
edited
Loading

@MylesBorins I'm sorry I completely lost track of this and never found the time to fix it. Please feel free to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.