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

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

Closed
wants to merge 49 commits into from
Closed

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

wants to merge 49 commits into from

Conversation

indutny
Copy link
Member

@indutny indutny commented May 5, 2021 •
edited

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 anna@addaleax.net
Reviewed-By: Rich Trott rtrott@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com

Backport-PR-URL: #38506

jasnell and others added 30 commits May 1, 2021 12:31
@jasnell @targos
assert: change status of legacy asserts
f65eadc 
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 <jasnell@gmail.com>

PR-URL: nodejs#38113
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@cjihrig @targos
deps: V8: cherry-pick 501482cbc704
8951c19 
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ä <marja@chromium.org>
    Commit-Queue: Marja Hölttä <marja@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#73800}

PR-URL: nodejs#38121
Fixes: nodejs#37978
Refs: v8/v8@501482cbc704
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@cjihrig @targos
test: add regression test for serdes readDouble()
aa0d814 
Refs: nodejs#37978
PR-URL: nodejs#38121
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@oyyd @targos
test: fix flaky test-zlib-unused-weak.js
e999da7 
PR-URL: nodejs#38149
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott @targos
test: remove unneeded m flag on regular expressions
552c945 
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@VoltrexKeyva @targos
doc: add link to V8
424c8e1 
Added the link redirecting to the V8 engine github repository just like
libuv.

PR-URL: nodejs#38144
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott @targos
test: skip test-hash-seed on armv6 and armv7
c7981da 
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 <rlau@redhat.com>
@Trott @targos
test: skip test-vm-memleak in ASAN
058abbe 
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 <rlau@redhat.com>
@ycjcl868 @targos
build: don't run test workflow on doc dir on macOS
5667d0a 
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 <rtrott@gmail.com>
@Trott @targos
test: skip fs.watch() test on IBMi
ce70ea8 
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 <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@srknzl @targos
doc: mention cryptographic prng in description of randomUUID
4350bf5 
PR-URL: nodejs#38074
Refs: nodejs#36729
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
doc: update links in ICU guide
c7c8722 
PR-URL: nodejs#38177
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@mscdex @targos
benchmark: improve compare.R output
c00c31c 
PR-URL: nodejs#38118
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@iguessitsokay @targos
doc: add command flag to import.meta.resolve
be28376 
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 <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Linkgoron @targos
doc: clarify child_process close event
bb8db84 
clarify the 'close' event description in the child_process docs.

fixes: nodejs#37998

PR-URL: nodejs#38181
Fixes: nodejs#37998
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Linkgoron @targos
test: add undefined fatalException exit code test
605f830 
Add a test that checks the exit code when _fatalException
is undefined

PR-URL: nodejs#38119
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Ayase-252 @targos
crypto: reduce range of size to int max
723977f 
Refs: nodejs#38090

PR-URL: nodejs#38096
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@eltociear @targos
test: fix typo in gtest-all.cc
95c7dab 
separater -> separator

PR-URL: nodejs#38224
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@tniessen @targos
test: fix typo in comment in binding.c
a4275ee 
PR-URL: nodejs#38220
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@iguessitsokay @targos
doc: standardize command flag notes
e6c599b 
remove italic styling from command flag notes

PR-URL: nodejs#38199
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@eladkishon @targos
util: add getSystemErrorMap() impl
08a6d9e 
PR-URL: nodejs#38101
Fixes: nodejs#37951
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Skn0tt @targos
typings: add JSDoc types to lib/path
e2c2f2b 
PR-URL: nodejs#38186
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Trott @targos
test: fix test-https-agent-additional-options.js
796007b 
`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 <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott @targos
test: add tests for missing https agent options
ae44e5f 
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 <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott @targos
doc: revise TLS minVersion/maxVersion text
5dfe5af 
Minor changes mostly to improve compliance with our style guide.

PR-URL: nodejs#38202
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@VoltrexKeyva @targos
typings: add JSDoc typings for assert
530e69e 
PR-URL: nodejs#38188
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@jasnell @targos
src: fix finalization crash
ec4be2d 
PR-URL: nodejs#38250
Fixes: nodejs#38040
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@VoltrexKeyva @targos
typings: add JSDoc typings for http
60c7591 
PR-URL: nodejs#38191
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@arkerone @targos
doc: change "oject" to "object"
9265027 
PR-URL: nodejs#38256
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Harshitha K P <harshitha014@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell @targos
test: fixup failing test/internet/test-dns.js
ea1183c 
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 <jasnell@gmail.com>

PR-URL: nodejs#38241
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
xavdid and others added 14 commits May 1, 2021 12:31
@xavdid @targos
typings: add JSDoc to os module functions
face4b8 
PR-URL: nodejs#38197
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@Trott @targos
test: fix flaky test-dns and test-dns-lookup
2e81ded 
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@XadillaX @targos
src: cache some context in locals
5ed770e 
Refs: nodejs@66566df

PR-URL: nodejs#37473
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott @targos
tools: skip macOS GitHub Actions test on doc-only changes
ed7c0d7 
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 <rlau@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
@aduh95 @targos
doc: harmonize changes list ordering
2e7f74c 
Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35454
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@Trott @targos
tools: bump remark-lint-preset-node to 1.17.1
cbb0a45 
Bump remark-lint-preset-node@1.16.0 to remark-lint-preset-node@1.17.1.

Refs: nodejs/remark-preset-lint-node#139

PR-URL: nodejs#35668
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@Trott @targos
tools: bump remark-lint-preset-node to 2.0.0
ac70a59 
Update remark@13.0.0, remark-lint-preset-node@2.0.0 and other
dependencies in the lint-md rollup.

PR-URL: nodejs#35905
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins @targos
tools: update ini in tools/node-lint-md-cli-rollup
c8f77f2 
Refs: nodejs#36473
Refs: GHSA-qqgx-2p2h-9c37

PR-URL: nodejs#36474
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos
tools: update all lint-md rollup dependencies
abd45d3 
PR-URL: nodejs#36843
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott @targos
tools: bump remark-present-lint-node from 2.0.0 to 2.0.1
5afaeaf 
This is a prerequisite for nodejs#37259.

PR-URL: nodejs#37270
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@Trott @targos
tools: update remark-preset-lint-node to 2.1.1
b76aa10 
PR-URL: nodejs#37604
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott @targos
tools: update glob-parent to 5.1.2
2c49953 
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 <duhamelantoine1995@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@Trott @targos
test: change Fixes: to Refs:
cd95bf3 
Tests don't fix things generally, so use "Refs:" to refer people to
GitHub issues.

PR-URL: nodejs#34568
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott @targos
test: move buffer-as-path symlink test to its own test file
93c917c 
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 <rexagod@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@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
This was referenced May 5, 2021
Merged
Closed
@indutny
Copy link
Member Author

indutny commented May 5, 2021 •
edited

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).

@indutny
node-api: faster threadsafe_function
9c9726b 
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 <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
Reviewers

@jasnell jasnell Awaiting requested review from jasnell

@Trott Trott Awaiting requested review from Trott

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet