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

build: build benchmark addons like test addons #29995

Merged
merged 1 commit into from Oct 17, 2019

Conversation

@richardlau
Copy link
Member

richardlau commented Oct 16, 2019

Build the addons for benchmarks in the same way that the addons for
tests are built.

Fixes: nodejs/build#1961
Refs: 53ca0b9#commitcomment-35494896

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
richardlau referenced this pull request Oct 16, 2019
Since worker threads are complete Node.js environments, including the
ability to load native addons, and since those native addons can
allocate resources to be freed when objects go out of scope, and since,
upon worker thread exit, the engine does not invoke the weak callbacks
responsible for freeing resources which still have references, this
modification introduces tracking for weak references such that a list
of outstanding weak references is maintained. This list is traversed
during environment teardown. The callbacks for the remaining weak
references are called.

This change is also relevant for Node.js embedder scenarios, because in
those cases the process also outlives the `node::Environment` and
therefore weak callbacks should also be rendered as environment cleanup
hooks to ensure proper cleanup after native addons. This changes
introduces the means by which this can be accomplished.

A benchmark is included which measures the time it takes to execute the
weak reference callback for a given number of weak references.

Re: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Oct 16, 2019

@nodejs-github-bot

This comment has been minimized.

@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Oct 16, 2019

@Trott
Trott approved these changes Oct 16, 2019
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Oct 16, 2019

I propose fast-tracking this to un-break nightly CI. Collaborators, please 👍 here if you agree. Thanks!

@Trott Trott added the fast-track label Oct 16, 2019
@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Oct 17, 2019

Going to step away from my computer for a bit.

Still waiting for https://ci.nodejs.org/job/node-test-commit-arm/26747/nodes=debian9-docker-armv7/ to complete (it was queued for four hours waiting for a free debian9-docker-armv7 host (at one point two of the three were disconnected) and has been running for 2+hours (and seems very slow)) but otherwise this looks landable assuming the remaining job comes back non-red.

Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: #29995
Fixes: nodejs/build#1961
Refs: 53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@richardlau richardlau force-pushed the richardlau:bench branch from d398cc5 to c8df5cf Oct 17, 2019
@richardlau richardlau self-assigned this Oct 17, 2019
@richardlau richardlau merged commit c8df5cf into nodejs:master Oct 17, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@richardlau

This comment has been minimized.

Copy link
Member Author

richardlau commented Oct 17, 2019

Landed in c8df5cf.

@richardlau richardlau deleted the richardlau:bench branch Oct 17, 2019
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Oct 22, 2019
Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: nodejs#29995
Fixes: nodejs/build#1961
Refs: nodejs@53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Oct 22, 2019
Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: nodejs#29995
Fixes: nodejs/build#1961
Refs: nodejs@53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: #29995
Fixes: nodejs/build#1961
Refs: 53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: #29995
Fixes: nodejs/build#1961
Refs: 53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 23, 2019
targos added a commit that referenced this pull request Nov 8, 2019
Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: #29995
Fixes: nodejs/build#1961
Refs: 53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
Build the addons for benchmarks in the same way that the addons for
tests are built.

PR-URL: #29995
Fixes: nodejs/build#1961
Refs: 53ca0b9#commitcomment-35494896
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.