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.

Sign up for GitHub

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

Jump to bottom

Begin AsyncWrap maintenance #3139

Closed
wants to merge 2 commits into from
Closed

Begin AsyncWrap maintenance #3139

wants to merge 2 commits into from

Conversation

trevnorris
Copy link
Contributor

This is long overdue maintenance on AsyncWrap. More PRs will follow, but segmenting to keep reviews simple.

R=@bnoordhuis
R=@indutny

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

@trevnorris trevnorris added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 30, 2015
@trevnorris
async_wrap: update providers and add test
71dc4c8 
Several provider ids have been removed that are no longer in use. Others
have been updated to match their class constructors.

Add test to ensure all internally listed providers are used.
@trevnorris
async-wrap: ensure all objects have internal field
66b2890 
If the constructor can't assign a class id then the heap snapshot will
not be able to report the object. So ensure that all AsyncWrap instances
use a FunctionTemplate instance with an internal field count >= 1.
@Fishrock123
Copy link
Contributor

More PRs will follow, but segmenting to keep reviews simple.

To be clear, async wrap will still work during this time, correct? Otherwise one PR may be more appropriate?

@trevnorris
Copy link
Contributor Author

@Fishrock123 Each PR will have a small set of fixes. No PR will prevent AsyncWrap from working at least as well as it does today.

This PR fixed outdated providers list and not all instances being reported in the heap snapshot.

@Fishrock123
Copy link
Contributor

Sounds good to me then. I definitely can't actually review this haha.

thefourtheye
thefourtheye reviewed Oct 1, 2015
View reviewed changes
src/async-wrap-inl.h
// Shift provider value over to prevent id collision.
persistent().SetWrapperClassId(NODE_ASYNC_ID_OFFSET + provider);
CHECK_NE(provider, PROVIDER_NONE);
CHECK_GE(object->InternalFieldCount(), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

These two will throw errors. Doesn't it make it major change? If this is an internal function, do we really need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't throw an error. It aborts. The first couldn't have happened anyway because of how the function signature works. It's there as a sanity check for future development. The latter should never have been happening as it would have caused issues when iterating the heap.

@thefourtheye
Copy link
Contributor

Can you please summarize what this PR does and why it is necessary?

Edit:

This PR fixed outdated providers list and not all instances being reported in the heap snapshot.

Ah I see. I'll try to find what providers are.

bnoordhuis
bnoordhuis reviewed Oct 1, 2015
View reviewed changes
src/env-inl.h
fn_t->SetClassName(FIXED_ONE_BYTE_STRING(isolate(), "InternalFieldObject"));
v8::Local<v8::ObjectTemplate> obj_t = fn_t->InstanceTemplate();
obj_t->SetInternalFieldCount(1);
set_generic_internal_field_template(obj_t);
Copy link
Member

Choose a reason for hiding this comment

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

Mild cognitive dissonance here, the _t suffix makes it look like the variables are typedefs at a quick glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot that convention. Was using it as shorthand for template. Will change.

@bnoordhuis
Copy link
Member

LGTM with some suggestions.

@Qard
Copy link
Member

Qard commented Oct 1, 2015

LGTM, other than the comments from Ben.

trevnorris added a commit that referenced this pull request Oct 1, 2015
@trevnorris
async_wrap: update providers and add test
e52864b 
Several provider ids have been removed that are no longer in use. Others
have been updated to match their class constructors.

Add test to ensure all internally listed providers are used.

PR-URL: #3139
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-by: Stephen Belanger <[email protected]>
trevnorris added a commit that referenced this pull request Oct 1, 2015
@trevnorris
async_wrap: ensure all objects have internal field
3f476ad 
If the constructor can't assign a class id then the heap snapshot will
not be able to report the object. So ensure that all AsyncWrap instances
use a FunctionTemplate instance with an internal field count >= 1.

PR-URL: #3139
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-by: Stephen Belanger <[email protected]>
@trevnorris
Copy link
Contributor Author

Landed with suggested changes in e52864b and 3f476ad.

Should land on current stable before going LTS.

@trevnorris trevnorris closed this Oct 1, 2015
trevnorris added a commit that referenced this pull request Oct 2, 2015
@trevnorris @rvagg
async_wrap: update providers and add test
99e6607 
Several provider ids have been removed that are no longer in use. Others
have been updated to match their class constructors.

Add test to ensure all internally listed providers are used.

PR-URL: #3139
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-by: Stephen Belanger <[email protected]>
trevnorris added a commit that referenced this pull request Oct 2, 2015
@trevnorris @rvagg
async_wrap: ensure all objects have internal field
39b8730 
If the constructor can't assign a class id then the heap snapshot will
not be able to report the object. So ensure that all AsyncWrap instances
use a FunctionTemplate instance with an internal field count >= 1.

PR-URL: #3139
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-by: Stephen Belanger <[email protected]>
@rvagg rvagg mentioned this pull request Oct 3, 2015
Closed
@trevnorris trevnorris deleted the async-awesome branch October 6, 2015 16:51
@MylesBorins
Copy link
Contributor

landed in v4.x-staging in e561585...39b8730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@trevnorris @Fishrock123 @thefourtheye @bnoordhuis @Qard @MylesBorins @indutny @jasnell