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

src: use unrefed async for GC tracking #16758

Closed
wants to merge 2 commits into from
Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 4, 2017

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 addaleax added the performance Issues and PRs related to the performance of Node.js. label Nov 4, 2017
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 4, 2017
@addaleax
Copy link
Member Author

addaleax commented Nov 4, 2017

@refack
Copy link
Contributor

refack commented Nov 4, 2017

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;

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍 🎉 ❤️

@refack
Copy link
Contributor

refack commented Nov 4, 2017

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
Copy link
Member

jasnell commented Nov 5, 2017

Niiiiice.

@jasnell
Copy link
Member

jasnell commented Nov 5, 2017

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

@joyeecheung
Copy link
Member

joyeecheung commented Nov 5, 2017

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
Copy link
Member

jasnell commented Nov 5, 2017

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

@@ -48,4 +48,6 @@ const kinds = [
}));
obs.observe({ entryTypes: ['gc'] });
global.gc();
// Keep the event loop alive to witness the GC happen.
setImmediate(() => {});
Copy link
Member

@joyeecheung joyeecheung Nov 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@refack
Copy link
Contributor

refack commented Nov 5, 2017

On all vs2015 builds:

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

@joyeecheung
Copy link
Member

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
Copy link
Member Author

addaleax commented Nov 5, 2017

╯°□°)╯︵ ┻━┻

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
Copy link
Contributor

refack commented Nov 5, 2017

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

refack commented Nov 11, 2017

@addaleax
Copy link
Member Author

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

@addaleax
Copy link
Member Author

addaleax commented Nov 11, 2017

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

@addaleax
Copy link
Member Author

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

@refack
Copy link
Contributor

refack commented Nov 11, 2017

Landed in 941c65b and 7b3446e

@refack refack closed this Nov 11, 2017
refack pushed a commit that referenced this pull request Nov 11, 2017
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>
refack pushed a commit that referenced this pull request Nov 11, 2017
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>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
evanlucas pushed a commit that referenced this pull request Nov 13, 2017
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>
@evanlucas evanlucas mentioned this pull request Nov 13, 2017
@MylesBorins
Copy link
Contributor

I've backported this to v8.x

Please lmk if you think it should bake longer

MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
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>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2017
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 addaleax deleted the fix-awu branch November 17, 2017 20:28
@gibfahn gibfahn mentioned this pull request Nov 21, 2017
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++. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants