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

[v14.x backport] node-api: faster threadsafe_function #38543

Closed
wants to merge 49 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented May 5, 2021

Invoke threadsafe_function during the same tick and avoid marshalling
costs between threads and/or churning event loop if either:

  1. There's a queued call already
  2. Push() is called while the main thread was running
    threadsafe_function

PR-URL: #38506
Reviewed-By: Anna Henningsen [email protected]
Reviewed-By: Rich Trott [email protected]
Reviewed-By: James M Snell [email protected]


Backport-PR-URL: #38506

jasnell and others added 30 commits May 1, 2021 12:31
Previously marked deprecated, but these are unlikely to ever see
breaking changes or complete removal. Mark as legacy instead.

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#38113
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Original commit message:

    Fix ValueDeserializer::ReadDouble() bounds check

    If end_ is smaller than sizeof(double), the result would wrap
    around, and lead to an invalid memory access.

    Refs: nodejs#37978
    Change-Id: Ibc8ddcb0c090358789a6a02f550538f91d431c1d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2801353
    Reviewed-by: Marja Hölttä <[email protected]>
    Commit-Queue: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#73800}

PR-URL: nodejs#38121
Fixes: nodejs#37978
Refs: v8/v8@501482cbc704
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Refs: nodejs#37978
PR-URL: nodejs#38121
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
PR-URL: nodejs#38149
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
The m flag has no effect on regular expressions that don't match the
start or the end of a line.

PR-URL: nodejs#38124
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Added the link redirecting to the V8 engine github repository just like
libuv.

PR-URL: nodejs#38144
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
The test is too slow to run on the Raspberry Pi bots. (It takes over 900
seconds to run on Pi 3 bots and even more than that on Pi 2 bots.) Skip
it on armv6 and armv7.

Refs: nodejs#34289

PR-URL: nodejs#34289
Reviewed-By: Richard Lau <[email protected]>
ASAN increases memory usage, which in turn messes up the memory leak
test. Skip the test on ASAN.

PR-URL: nodejs#34289
Reviewed-By: Richard Lau <[email protected]>
Leaving it in the Linux workflow because addons tests are affected by
changes to addons.md example code. So we need to keep that running
somewhere for docs changes, but one platform seems sufficient.

PR-URL: nodejs#37999
Reviewed-By: Rich Trott <[email protected]>
We've added pummel tests to CI, so we need to add a skip for IBMi for
the fs.watch() test there as we have for all the fs.watch() tests
elsewhere.

PR-URL: nodejs#38192
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#38177
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#38118
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
import.meta.resolve is only available under
--experimental-import-meta-resolve cli flag.

Source:
https://github.com/nodejs/node/blob/e46c680bf2b211bbd52cf959ca17ee98c7f657f5/lib/internal/modules/esm/translators.js#L132

PR-URL: nodejs#38171
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
clarify the 'close' event description in the child_process docs.

fixes: nodejs#37998

PR-URL: nodejs#38181
Fixes: nodejs#37998
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Add a test that checks the exit code when _fatalException
is undefined

PR-URL: nodejs#38119
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Refs: nodejs#38090

PR-URL: nodejs#38096
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
separater -> separator

PR-URL: nodejs#38224
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#38220
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
remove italic styling from command flag notes

PR-URL: nodejs#38199
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
PR-URL: nodejs#38101
Fixes: nodejs#37951
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#38186
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
`value` was always being assigned to an `undefined` property of an
Array. Thus, the assertions that depended on `value` being defined were
never being checked. Assign `value` the correct...er...value.

PR-URL: nodejs#38202
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Add test that minVersion and maxVersion options are accepted. This
should complete coverage for lib/https.js.

Refs: https://codecov.io/gh/nodejs/node/src/ec0dcd720e10831b3e783b415c5dc011ed5be2f8/lib/https.js

PR-URL: nodejs#38202
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Minor changes mostly to improve compliance with our style guide.

PR-URL: nodejs#38202
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#38188
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#38250
Fixes: nodejs#38040
Reviewed-By: Beth Griggs <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#38191
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#38256
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Harshitha K P <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
The `ttl` for the `nodejs.org` DNS record is returning `0`
currently. The test checks for `> 0`, causing the test to
fail.

Signed-off-by: James M Snell <[email protected]>

PR-URL: nodejs#38241
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
xavdid and others added 14 commits May 1, 2021 12:31
PR-URL: nodejs#38197
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Ubuntu 16.04 is going to be unsupported after April 2021. Switching
Node.js CI to Ubuntu 18.04 for the internet tests resulted in failures
in test-dns and test-dns-lookup because it returns server failure/try
again on .invalid domain. Add a hostname for testing that will return
record not found.

PR-URL: nodejs#38282
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Refs: nodejs@66566df

PR-URL: nodejs#37473
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
We skip macOS GitHub Actions pushes on doc-only changes but the
intention was no doubt to skip them on pull requests too. We still run
the tests on Linux, so addons tests will still run when addons docs
change.

PR-URL: nodejs#38296
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ujjwal Sharma <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Update [email protected], [email protected] and other
dependencies in the lint-md rollup.

PR-URL: nodejs#35905
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Shelley Vohr <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#36843
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
This is a prerequisite for nodejs#37259.

PR-URL: nodejs#37270
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Beth Griggs <[email protected]>
PR-URL: nodejs#37604
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
In the markdown linting rollup script, update glob-parent to 5.1.2.

Refs: https://app.snyk.io/vuln/SNYK-JS-GLOBPARENT-1016905

PR-URL: nodejs#37646
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Tests don't fix things generally, so use "Refs:" to refer people to
GitHub issues.

PR-URL: nodejs#34568
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
The buffer-as-path test for fs.symlinkSync() is a bit unusual and
potentially error-prone embedded in the general fs.symlink() test. Move
it to its own test file.

Refs: https://github.com/nodejs/node/pull/34540/files#r463168354

PR-URL: nodejs#34569
Reviewed-By: Pranshu Srivastava <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@indutny indutny requested review from jasnell, Trott and addaleax May 5, 2021 01:44
@github-actions github-actions bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v14.x labels May 5, 2021
@indutny
Copy link
Member Author

indutny commented May 5, 2021

The linux failures seems unrelated to files changed here. It fails on node_buffer.cc warnings, but this PR doesn't touch it.

@targos
Copy link
Member

targos commented May 5, 2021

The commit message should be identical to the original one ( we add backport PR URL when it lands).

Invoke threadsafe_function during the same tick and avoid marshalling
costs between threads and/or churning event loop if either:

1. There's a queued call already
2. `Push()` is called while the main thread was running
   threadsafe_function

PR-URL: nodejs#38506
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@indutny
Copy link
Member Author

indutny commented May 5, 2021

@targos oops. Fixed now. Thanks!

@targos
Copy link
Member

targos commented May 30, 2021

It just landed cleanly on v14.x-staging.

@targos targos closed this May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.