src: use unrefed async for GC tracking by addaleax · Pull Request #16758 · nodejs/node

addaleax · 2017-11-04T21:34:21Z

Do not let an internal handle keep the event loop alive.

I think this patch is okay since GC isn’t deterministic anyway.

Fixes: #16210

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

@Trott @refack @jasnell

addaleax · 2017-11-04T21:38:56Z

CI: https://ci.nodejs.org/job/node-test-commit/13738/
Stress test, linuxone: https://ci.nodejs.org/job/node-stress-single-test/1547/nodes=rhel72-s390x/

refack · 2017-11-04T21:57:14Z

IMHO the unref makes sense since if there are any live PerformanceObserver they will keep the loop alive anyway. If there are none, that uv_async_t is effectively a noop.

Alternative resolution:

auto observers = env->performance_state()->observers;
if (observers == nullptr || arraysize(observers) = 0) return;

Trott

😍 🎉 ❤️

refack · 2017-11-04T22:11:39Z

if there are any live PerformanceObserver they will keep the loop alive

wait... they won't. So this becomes a little delicate.
@jasnell If there are observers, should a GC event revive the loop, or is it insignificant since the process will exit ASAP?

jasnell · 2017-11-05T01:36:10Z

Niiiiice.

jasnell · 2017-11-05T01:36:50Z

PerformanceObservers are passive. They won't keep anything open.

joyeecheung · 2017-11-05T01:42:00Z

On ~~all~~ some vs2015 builds:

not ok 302 parallel/test-performance-gc
  ---
  duration_ms: 0.140
  severity: fail
  stack: |-
    Mismatched <anonymous> function calls. Expected at least 1, actual 0.
        at Object.exports.mustCallAtLeast (c:\workspace\node-test-binary-windows\test\common\index.js:494:10)
        at Object.<anonymous> (c:\workspace\node-test-binary-windows\test\parallel\test-performance-gc.js:34:46)
        at Module._compile (module.js:641:30)
        at Object.Module._extensions..js (module.js:652:10)
        at Module.load (module.js:560:32)
        at tryModuleLoad (module.js:503:12)
        at Function.Module._load (module.js:495:3)
        at Function.Module.runMain (module.js:682:10)
        at startup (bootstrap_node.js:191:16)
  ...

jasnell · 2017-11-05T01:45:18Z

Make the 'setImmediate' in that test a 'setInterval' I think.

joyeecheung · 2017-11-05T01:45:52Z

test/parallel/test-performance-gc.js

hmmm, some how this is not enough for a gc hook..

EDIT: let's go with #16758 (comment) ?

refack · 2017-11-05T01:47:16Z

On all vs2015 builds:

All Windows fail (if it was just VS2015 I'd plotz)

joyeecheung · 2017-11-05T05:47:31Z

During the initial investigation I think this only reproduced on vs2015/vcbt2015 builds? Or we might just missed other 2017 builds at that time.
#16210 (comment)

Curious what's differed about those builds...

addaleax · 2017-11-05T10:20:08Z

╯°□°）╯︵ ┻━┻

Make the 'setImmediate' in that test a 'setInterval' I think.

As much as I hate to say it … isn’t that just masking a problem instead of taking the time to understand what’s going on? 😞

At least this seems to be failing consistently, yay for that.

refack · 2017-11-05T15:04:57Z

During the initial investigation I think this only reproduced on vs2015/vcbt2015 builds? Or we might just missed other 2017 builds at that time.
#16210 (comment)

Curious what's differed about those builds...

I think the issue was computer related more then compiler related. But we do have a different flake that happens only on VS2017 - #15558

addaleax · 2017-11-10T22:23:32Z

Okay, picking up work on this again…

Fresh CI: https://ci.nodejs.org/job/node-test-commit/13920/

Do not let an internal handle keep the event loop alive. Fixes: nodejs#16210

This effectively reverts 5c475a7 and 75095d7.

addaleax · 2017-11-11T13:48:31Z

Looks like using 2 nested setImmediate()s does the trick, so it’s probably a difference in how uv_async_t is implemented on windows.

Jenkins is hanging, otherwise I’d start CI, this should be ready…

refack · 2017-11-11T14:57:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11360/

addaleax · 2017-11-11T14:58:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11361/

addaleax · 2017-11-11T15:02:34Z

@refack Sorry, didn’t see that! 😄 But probably a good idea to make sure nothing’s flaky :)

addaleax · 2017-11-11T16:33:07Z

Okay, CI seems good - feel free to land this,otherwise I'll do that tomorrow

refack · 2017-11-11T16:51:49Z

Landed in 941c65b and 7b3446e

Do not let an internal handle keep the event loop alive. PR-URL: #16758 Fixes: https://github.com/node/issues/16210 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

This effectively reverts 5c475a7 and 75095d7. PR-URL: #16758 Fixes: https://github.com/node/issues/16210 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Do not let an internal handle keep the event loop alive. PR-URL: #16758 Fixes: https://github.com/node/issues/16210 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

This effectively reverts 5c475a7 and 75095d7. PR-URL: #16758 Fixes: https://github.com/node/issues/16210 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

MylesBorins · 2017-11-17T16:08:52Z

I've backported this to v8.x

Please lmk if you think it should bake longer

Do not let an internal handle keep the event loop alive. PR-URL: #16758 Fixes: https://github.com/node/issues/16210 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

This effectively reverts 5c475a7 and 75095d7. PR-URL: #16758 Fixes: https://github.com/node/issues/16210 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

addaleax added the performance Issues and PRs related to the performance of Node.js. label Nov 4, 2017

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 4, 2017

refack approved these changes Nov 4, 2017

View reviewed changes

addaleax mentioned this pull request Nov 4, 2017

test: add simple extra debugging to flaky test (async-wrap-uncaughtexception) #16757

Closed

3 tasks

Trott approved these changes Nov 4, 2017

View reviewed changes

cjihrig approved these changes Nov 4, 2017

View reviewed changes

jasnell approved these changes Nov 5, 2017

View reviewed changes

joyeecheung reviewed Nov 5, 2017

View reviewed changes

benjamingr approved these changes Nov 5, 2017

View reviewed changes

addaleax force-pushed the fix-awu branch from 61732d7 to 7038a79 Compare November 10, 2017 22:22

addaleax added 2 commits November 11, 2017 14:39

src: use unrefed async for GC tracking

808887d

Do not let an internal handle keep the event loop alive. Fixes: nodejs#16210

test: unmark flaky test

26494ab

This effectively reverts 5c475a7 and 75095d7.

addaleax force-pushed the fix-awu branch from 7038a79 to 26494ab Compare November 11, 2017 13:40

refack closed this Nov 11, 2017

evanlucas mentioned this pull request Nov 13, 2017

v9.2.0 proposal #16992

Merged

MylesBorins added dont-land-on-v6.x labels Nov 17, 2017

addaleax deleted the fix-awu branch November 17, 2017 20:28

gibfahn mentioned this pull request Nov 21, 2017

v8.9.2 proposal #17204

Merged

Uh oh!

Conversation

addaleax commented Nov 4, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Nov 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Nov 4, 2017

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 4, 2017

Uh oh!

jasnell commented Nov 5, 2017

Uh oh!

jasnell commented Nov 5, 2017

Uh oh!

joyeecheung commented Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Nov 5, 2017

Uh oh!

joyeecheung Nov 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 5, 2017

Uh oh!

joyeecheung commented Nov 5, 2017

Uh oh!

addaleax commented Nov 5, 2017

Uh oh!

refack commented Nov 5, 2017

Uh oh!

addaleax commented Nov 10, 2017

Uh oh!

addaleax commented Nov 11, 2017

Uh oh!

refack commented Nov 11, 2017

Uh oh!

addaleax commented Nov 11, 2017

Uh oh!

addaleax commented Nov 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 11, 2017

Uh oh!

refack commented Nov 11, 2017

Uh oh!

MylesBorins commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

addaleax commented Nov 4, 2017 •

edited

Loading

joyeecheung commented Nov 5, 2017 •

edited

Loading

joyeecheung Nov 5, 2017 •

edited

Loading

addaleax commented Nov 11, 2017 •

edited

Loading