FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Begin AsyncWrap maintenance by trevnorris · Pull Request #3139 · nodejs/node · GitHub

/ node Public

Begin AsyncWrap maintenance - #3139

Closed
trevnorris wants to merge 2 commits into
nodejs:masterfrom
trevnorris:async-awesome
ConversationCommits (2)ChecksFiles changed
Closed

Begin AsyncWrap maintenance#3139
trevnorris wants to merge 2 commits into
nodejs:masterfrom
trevnorris:async-awesome

Conversation

trevnorris commented Sep 30, 2015

Copy link
Copy Markdown
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 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 30, 2015
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.
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 commented Sep 30, 2015

Copy link
Copy Markdown
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 commented Oct 1, 2015

Copy link
Copy Markdown
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 commented Oct 1, 2015

Copy link
Copy Markdown
Contributor

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

thefourtheye reviewed Oct 1, 2015
View reviewed changes
Comment thread src/async-wrap-inl.h

thefourtheye Oct 1, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

trevnorris Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Oct 1, 2015

Copy link
Copy Markdown
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 reviewed Oct 1, 2015
View reviewed changes
Comment thread src/env-inl.h

bnoordhuis Oct 1, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

trevnorris Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis commented Oct 1, 2015

Copy link
Copy Markdown
Member

LGTM with some suggestions.

Qard commented Oct 1, 2015

Copy link
Copy Markdown
Member

LGTM, other than the comments from Ben.

trevnorris added a commit that referenced this pull request Oct 1, 2015
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 <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Stephen Belanger <admin@stephenbelanger.com>
trevnorris added a commit that referenced this pull request Oct 1, 2015
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 <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Stephen Belanger <admin@stephenbelanger.com>

trevnorris commented Oct 1, 2015

Copy link
Copy Markdown
Contributor Author

Landed with suggested changes in e52864b and 3f476ad.

Should land on current stable before going LTS.

trevnorris closed this Oct 1, 2015
trevnorris added a commit that referenced this pull request Oct 2, 2015
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 <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Stephen Belanger <admin@stephenbelanger.com>
trevnorris added a commit that referenced this pull request Oct 2, 2015
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 <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-by: Stephen Belanger <admin@stephenbelanger.com>
rvagg mentioned this pull request Oct 3, 2015
Closed
trevnorris deleted the async-awesome branch October 6, 2015 16:51

MylesBorins commented Nov 17, 2015

Copy link
Copy Markdown
Contributor

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

Back | FazBrowse Home | New Git URL