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

async_hooks: fix Promises with later enabled hooks #13242

Closed
wants to merge 6 commits into from

Conversation

addaleax
Copy link
Member

Based on #13224

Fixes: #13237

/cc @AndreasMadsen @matthewloring @Fishrock123 @nodejs/diagnostics

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

async_hooks

@addaleax addaleax added async_hooks Issues and PRs related to the async hooks subsystem. async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels May 26, 2017
@addaleax addaleax added this to the 8.0.0 milestone May 26, 2017
@nodejs-github-bot nodejs-github-bot added async_wrap build Issues and PRs related to build files or the CI. c++ Issues and PRs that require attention from people who are familiar with C++. labels May 26, 2017
@addaleax addaleax removed the build Issues and PRs related to build files or the CI. label May 26, 2017
if (type != PromiseHookType::kInit) {
// Do not emit an init event.
stored_init_field = env->async_hooks()->fields()[AsyncHooks::kInit];
env->async_hooks()->fields()[AsyncHooks::kInit] = 0;
Copy link
Member

Choose a reason for hiding this comment

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

well, this is a fun hack :) – I'm not sure I approve, would a default parameter in AsyncWrap::AsyncWrap and AsyncWrap::AsyncReset be that difficult?

@Fishrock123 thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

would a default parameter in AsyncWrap::AsyncWrap and AsyncWrap::AsyncReset be that difficult?

@AndreasMadsen yeah no you’re right. done. 😄

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM.

Although, I do think we should use

uint32_t* fields_ptr = async_hooks->fields();
if (fields_ptr[AsyncHooks::kInit] +
    fields_ptr[AsyncHooks::kBefore] +
    fields_ptr[AsyncHooks::kAfter] +
    fields_ptr[AsyncHooks::kDestroy] > 0) {

}

instead of #13177 for a more consistent performance, when createHook() has been called but no hooks are enabled. But that is not relevant in this PR.

@addaleax
Copy link
Member Author

@AndreasMadsen Yes, I see no problem with doing that too; but I would really like to keep the SetPromiseHook() call lazy so that non-async_hooks users don’t pay the price for it.

@AndreasMadsen
Copy link
Member

AndreasMadsen commented May 26, 2017

Yes, I see no problem with doing that too; but I would really like to keep the SetPromiseHook() call lazy so that non-async_hooks users don’t pay the price for it.

Makes sense.

edit: the goal is that there is no difference between calling .disable() and never having called require('async_hooks'), but that is not a trivial task and is likely over-optimistic.

@matthewloring
Copy link

This looks good on my end. Sorry for the lack of bandwidth to address this on #13224 directly.

@addaleax
Copy link
Member Author

@matthewloring No problem! This PR is me cleaning up my own mistake. If you want you can still cherry-pick 0ec3eba0534f7de36c6c2e8baa38188a5279727f into your PR so that it can land a bit earlier.

@matthewloring
Copy link

As long as it gets in to node 8 I'm happy either way :)

@addaleax
Copy link
Member Author

the goal is that there is no difference between calling .disable() and never having called require('async_hooks'), but that is not a trivial task and is likely over-optimistic

It shouldn’t be so terribly hard either. Maybe I can take a look after the current bunch of async_wrap PRs are through.

As long as it gets in to node 8 I'm happy either way :)

@matthewloring Yeah, me too. Maybe we should ask @nodejs/collaborators whether they think it’s a good idea to fast-track this? At this point you and @AndreasMadsen seem to be “the” most relevant reviewers for async_hooks by far… (speaking of which, would you mind giving #13142 a look? 😬)

Matt Loring and others added 4 commits May 27, 2017 12:28
Promises do not have any internal fields by default. V8 recently added
the capability of configuring the number of internal fields on promises.
This change adds an internal field to promises allowing promises to be
wrapped directly by the PromiseWrap object. In addition to cleaner code
this avoids an extra object allocation per promise and speeds up promise
creation with async_hooks enabled by ~2x.
Assign a `PromiseWrap` instance to Promises that do not have one
yet when the PromiseHook is being called.

Fixes: nodejs#13237
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants