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

worker: add ability to take heap snapshot from parent thread #31569

Closed
wants to merge 6 commits into from

Conversation

@addaleax
Copy link
Member

addaleax commented Jan 29, 2020

@nodejs/workers

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
@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Jan 29, 2020

Add a test for when the worker is no longer running?

@addaleax addaleax force-pushed the addaleax:worker-heapdump branch Jan 29, 2020
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Jan 29, 2020

@richardlau Thanks, yes – I had missed that I hadn’t fully implemented that part yet 👍

@vdeturckheim vdeturckheim dismissed their stale review Jan 29, 2020

miss-click sorry about that

doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/worker_threads.md Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
lib/internal/heap_utils.js Show resolved Hide resolved
lib/internal/errors.js Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@awibox
awibox approved these changes Jan 30, 2020
@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Feb 1, 2020
@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 1, 2020

Needs a rebase. Also is the failure of test.parallel/test-bootstrap-modules CI when run with tools/test.py --worker related to this change? I haven't seen it before and we got it three times in a row here.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Feb 1, 2020

Needs a rebase. Also is the failure of test.parallel/test-bootstrap-modules CI when run with tools/test.py --worker related to this change? I haven't seen it before and we got it three times in a row here.

Yes, it is related:

21:56:18 not ok 112 parallel/test-bootstrap-modules
21:56:18   ---
21:56:18   duration_ms: 0.278
21:56:18   severity: fail
21:56:18   exitcode: 1
21:56:18   stack: |-
21:56:18     
21:56:18     events.js:298
21:56:18           throw er; // Unhandled 'error' event
21:56:18           ^
21:56:18     AssertionError [ERR_ASSERTION]: These modules were unexpectedly loaded:
21:56:18       Internal Binding stream_wrap,
21:56:18       Internal Binding uv,
21:56:18       NativeModule internal/heap_utils,
21:56:18       NativeModule internal/stream_base_commons
21:56:18     
21:56:18         at Object.<anonymous> (/home/iojs/build/workspace/node-test-commit-custom-suites-freestyle/test/parallel/test-bootstrap-modules.js:131:8)
21:56:18         at Module._compile (internal/modules/cjs/loader.js:1208:30)
21:56:18         at Object.Module._extensions..js (internal/modules/cjs/loader.js:1228:10)
...

test-bootstrap-modules is there to alert when extra modules are being loaded by the bootstrap process, which they are for workers in this PR, which has added internal/heap_utils and requires it from internal/worker. internal/heap_utils requires internal/stream_base_commons and that requires stream_wrap and uv (

const {
WriteWrap,
kReadBytesOrError,
kArrayBufferOffset,
kBytesWritten,
kLastWriteWasAsync,
streamBaseState
} = internalBinding('stream_wrap');
const { UV_EOF } = internalBinding('uv');
).

If we're okay adding these extra modules to the bootstrapping of worker threads (it's probably okay -- the whole point of the test is to make us make a considered decision) then they should be added to the list of modules expected to be loaded for workers in the test:

if (!common.isMainThread) {
expectedModules.add('Internal Binding messaging');
expectedModules.add('Internal Binding symbols');
expectedModules.add('Internal Binding worker');
expectedModules.add('NativeModule _stream_duplex');
expectedModules.add('NativeModule _stream_passthrough');
expectedModules.add('NativeModule _stream_readable');
expectedModules.add('NativeModule _stream_transform');
expectedModules.add('NativeModule _stream_writable');
expectedModules.add('NativeModule internal/error-serdes');
expectedModules.add('NativeModule internal/process/worker_thread_only');
expectedModules.add('NativeModule internal/streams/buffer_list');
expectedModules.add('NativeModule internal/streams/destroy');
expectedModules.add('NativeModule internal/streams/end-of-stream');
expectedModules.add('NativeModule internal/streams/legacy');
expectedModules.add('NativeModule internal/streams/pipeline');
expectedModules.add('NativeModule internal/streams/state');
expectedModules.add('NativeModule internal/worker');
expectedModules.add('NativeModule internal/worker/io');
expectedModules.add('NativeModule stream');
expectedModules.add('NativeModule worker_threads');
}

otherwise perhaps internal/heap_utils should be lazily loaded.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 1, 2020

Codecov Report

Merging #31569 into master will increase coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #31569      +/-   ##
==========================================
+ Coverage   96.16%    97.3%   +1.13%     
==========================================
  Files         193      193              
  Lines       64651    64653       +2     
==========================================
+ Hits        62172    62910     +738     
+ Misses       2479     1743     -736
Impacted Files Coverage Δ
lib/_http_server.js 98.16% <0%> (-0.12%) ⬇️
lib/internal/process/per_thread.js 98.62% <0%> (ø) ⬆️
lib/internal/http2/core.js 96.51% <0%> (+0.12%) ⬆️
lib/internal/fs/utils.js 97.2% <0%> (+0.29%) ⬆️
lib/child_process.js 100% <0%> (+0.57%) ⬆️
lib/internal/child_process/serialization.js 93.49% <0%> (+0.81%) ⬆️
lib/internal/streams/async_iterator.js 99.12% <0%> (+0.85%) ⬆️
lib/internal/url.js 98.68% <0%> (+1.17%) ⬆️
lib/internal/child_process.js 97.23% <0%> (+1.29%) ⬆️
lib/net.js 98.46% <0%> (+1.41%) ⬆️
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2272489...6bbff20. Read the comment docs.

addaleax added 6 commits Jan 29, 2020
@addaleax addaleax force-pushed the addaleax:worker-heapdump branch to 6bbff20 Feb 1, 2020
@addaleax

This comment has been minimized.

Copy link
Member Author

addaleax commented Feb 1, 2020

Needs a rebase.

Done! :)

otherwise perhaps internal/heap_utils should be lazily loaded.

Yeah, I think that makes sense here. Done! 👍

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 3, 2020

Landed in 875a4d1

@Trott Trott closed this Feb 3, 2020
Trott added a commit that referenced this pull request Feb 3, 2020
PR-URL: #31569
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax addaleax deleted the addaleax:worker-heapdump branch Feb 4, 2020
codebytere added a commit that referenced this pull request Feb 17, 2020
PR-URL: #31569
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Feb 17, 2020
codebytere added a commit that referenced this pull request Feb 18, 2020
Notable changes:

* async_hooks
  * add executionAsyncResource (Matteo Collina) #30959
* crypto
  * add crypto.diffieHellman (Tobias Nießen) #31178
  * add DH support to generateKeyPair (Tobias Nießen) #31178
  * simplify DH groups (Tobias Nießen) #31178
  * add key type 'dh' (Tobias Nießen) #31178
* test
  * skip keygen tests on arm systems (Tobias Nießen) #31178
* perf_hooks
  * add property flags to GCPerformanceEntry (Kirill Fomichev) #29547
* process
  * report ArrayBuffer memory in `memoryUsage()` (Anna Henningsen) #31550
* readline
  * make tab size configurable (Ruben Bridgewater) #31318
* report
  * add support for Workers (Anna Henningsen) #31386
* worker
  * add ability to take heap snapshot from parent thread (Anna Henningsen) #31569
* added new collaborators
  * add ronag to collaborators (Robert Nagy) #31498

PR-URL: #31837
codebytere added a commit that referenced this pull request Feb 18, 2020
Notable changes:

* async_hooks
  * add executionAsyncResource (Matteo Collina) #30959
* crypto
  * add crypto.diffieHellman (Tobias Nießen) #31178
  * add DH support to generateKeyPair (Tobias Nießen) #31178
  * simplify DH groups (Tobias Nießen) #31178
  * add key type 'dh' (Tobias Nießen) #31178
* test
  * skip keygen tests on arm systems (Tobias Nießen) #31178
* perf_hooks
  * add property flags to GCPerformanceEntry (Kirill Fomichev) #29547
* process
  * report ArrayBuffer memory in `memoryUsage()` (Anna Henningsen) #31550
* readline
  * make tab size configurable (Ruben Bridgewater) #31318
* report
  * add support for Workers (Anna Henningsen) #31386
* worker
  * add ability to take heap snapshot from parent thread (Anna Henningsen) #31569
* added new collaborators
  * add ronag to collaborators (Robert Nagy) #31498

PR-URL: #31837
@hyj1991 hyj1991 mentioned this pull request Feb 21, 2020
@Flarna Flarna mentioned this pull request Mar 3, 2020
3 of 3 tasks complete
mmarchini added a commit that referenced this pull request Mar 10, 2020
Adapt doc to match implementation which exports getHeapSnapshot().

PR-URL: #32061
Refs: #31569
Refs: https://github.com/nodejs/node/blob/987a67339518d0380177a2e589f2bbd274230d0e/lib/internal/worker.js#L323
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins added a commit that referenced this pull request Mar 10, 2020
Adapt doc to match implementation which exports getHeapSnapshot().

PR-URL: #32061
Refs: #31569
Refs: https://github.com/nodejs/node/blob/987a67339518d0380177a2e589f2bbd274230d0e/lib/internal/worker.js#L323
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.