async-wrap: add provider id and object info cb by trevnorris · Pull Request #1896 · nodejs/node

trevnorris · 2015-06-04T16:34:54Z

Re-add the wrapper class id to AsyncWrap instances so they can be
tracked directly in a heapdump.

Previously the class id was given without setting the heap dump wrapper
class info provider. Causing a segfault when a heapdump was taken. This
has been added, and the label_ set to the given provider name so each
instance can be identified.

R=@bnoordhuis

For performance testing I ran the following:

varJSStream=process.binding('js_stream').JSStream;varITER=1e7;vart=process.hrtime();for(vari=0;i<ITER;i++)newJSStream();t=process.hrtime(t);console.log(((t[0]*1e9+t[1])/ITER).toFixed(1)+' ns/op');

It was the most direct way I found to instantiate a new AsyncWrap instance from JS. It shows no performance degradation with this patch applied.

trevnorris · 2015-06-04T16:36:22Z

Here I'd prefer to unwrap the instance as the given provider type that's passed, but am coming up short on the necessary syntax.

guess I could do a massive switch statement, but that feels wrong.

ofrobots · 2015-06-04T16:51:27Z

I'm feeling a little bit un-easy about the magic Class Ids.

Is there a deterministic way to generate a class id? that's pretty much how it's done everywhere else in core.

smalloc is the only other example I can find. We could maintain a central table for classIds. The danger that I am thinking about is if user-space modules need to use use classIds as well and happen to use the same value. It would be nice if one (i.e. user-space or core) could request a classId from the table. That way we can be sure that there won't be collisions.
I am only 'a little bit un-easy', and don't have objections to this PR landing as is.

Good idea. Think that's definitely a PR to explore.

trevnorris · 2015-06-04T17:44:26Z

I'd prefer to just run SetWrapperClassInfoProvider() from node.cc, but then I'd have to migrate all the RetainedAsyncInfo logic as well. Thoughts on a better solution?

Can't you export a function in src/async-wrap.cc and call that in src/node.cc?

Oop. Missed this comment. Sounds like a much better alternative.

bnoordhuis · 2015-06-06T11:52:45Z

I see two issues with this PR.

It reports the wrong size for instances, it's always sizeof(AsyncWrap). That's wrong in general and very wrong for variable length types like WriteWrap.
It heap-allocates a new RetainedAsyncInfo for every instance and that:

2a. Wastes memory because most instances have a static layout and size.

2b. Probably slows down heap snapshots. RetainedAsyncInfo::IsEquivalent() only considers RetainedAsyncInfo instances equivalent when they point to the same AsyncWrap instance instead of instances of the same class. The heap snapshot generator has to do more work because of that although I don't know exactly how much more.

2a and 2b probably aren't that significant so long as there aren't too many AsyncWrap instances but 1 most definitely is.

trevnorris · 2015-06-08T16:31:13Z

@bnoordhuis I should have labeled this as WIP. Knew before it was posted that the implementation was incomplete.

(1) was my biggest concern while writing this patch, and something I couldn't figure out on my own. Was hoping you'd have a suggestion about how to potentially get around it.

Wasn't aware of the issues pointed out in (2). Most of the implementation was pulled directly out of smalloc.cc, which originally came from your implementation in 6a128e0. I'll be more than happy to work on both those points if you think other issues in this PR can be worked out.

bnoordhuis · 2015-06-08T16:38:32Z

I think the easiest solution to (1) is to add a virtual size_t self_size() const = 0; pure virtual method to AsyncWrap and have descendants implement that. For most, return sizeof(*this); will suffice.

Regarding (2), it would help to have static RetainedAsyncInfo instances for statically sized classes but that can be done as a future optimization.

trevnorris · 2015-06-08T23:05:38Z

@bnoordhuis Thanks for the reviews. Finished fixing the last round.

While I've been wanting a means to do this (furthering use of AsyncWrap and all that), the patch became more invasive then I imaged it would be. If you can think of a more graceful implementation I'll redo this from scratch if necessary.

trevnorris · 2015-06-09T22:50:48Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/790/

trevnorris · 2015-06-10T05:06:14Z

All test failures are the same:

not ok 105 - test-cluster-worker-wait-server-close.js

I doubt it's related to this PR, but don't know for sure.

jbergstroem · 2015-06-10T05:35:12Z

@trevnorris the test seems to time out as well.

trevnorris · 2015-06-10T06:01:30Z

@jbergstroem Thank for pointing that out.

Olegas · 2015-06-10T06:13:22Z

I know this test. It's mine. It was kinda broken while #1400 is merged.

trevnorris · 2015-06-10T23:14:49Z

Started new CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/798/

trevnorris · 2015-06-11T02:36:08Z

And again. Something went strange w/ my force push last time.

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/799/

trevnorris · 2015-06-11T04:36:54Z

CI completed. Same failures as before. None related to this patch.

@bnoordhuis if you're okay with the fixes, mind if I merge this?

bnoordhuis · 2015-06-14T22:06:58Z

Magic constant.

trevnorris · 2015-06-15T21:40:25Z

@bnoordhuis Thanks much for the additional review. All points have been addressed.

Re-add the wrapper class id to AsyncWrap instances so they can be tracked directly in a heapdump. Previously the class id was given without setting the heap dump wrapper class info provider. Causing a segfault when a heapdump was taken. This has been added, and the label_ set to the given provider name so each instance can be identified. The id will not be set of the passed object has no internal field count. As the class pointer cannot be retrieved from the object. In order to properly report the allocated size of each class, the new pure virtual method self_size() has been introduces.

trevnorris · 2015-06-17T18:08:24Z

CI: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/41/

bnoordhuis · 2015-06-17T18:13:26Z

LGTM at a glance.

trevnorris · 2015-06-17T18:55:35Z

Getting the following from centos5-32:

not ok 532 - test-process-argv-0.js
# argv=["/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/out/Release/iojs","/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/test/parallel/test-process-argv-0.js"]
# exec="/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/out/Release/iojs"
# CHILD: argv=["/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/out/Release/iojs","/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/test/parallel/test-process-argv-0.js","child"]
# # assert.js:89
# throw new assert.AssertionError({
# ^
# AssertionError: '' == '/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/out/Release/iojs'
# at ChildProcess.<anonymous> (/home/iojs/build/workspace/iojs+pr+linux/nodes/centos5-32/test/parallel/test-process-argv-0.js:26:12)
# at emitTwo (events.js:87:13)
# at ChildProcess.emit (events.js:172:7)
# at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)

Don't know how it could be related, so going to say that tests are looking good.

Re-add the wrapper class id to AsyncWrap instances so they can be tracked directly in a heapdump. Previously the class id was given without setting the heap dump wrapper class info provider. Causing a segfault when a heapdump was taken. This has been added, and the label_ set to the given provider name so each instance can be identified. The id will not be set of the passed object has no internal field count. As the class pointer cannot be retrieved from the object. In order to properly report the allocated size of each class, the new pure virtual method self_size() has been introduces. PR-URL: #1896 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

trevnorris · 2015-06-17T19:02:06Z

Landed in e56758a.

Thanks @bnoordhuis for the reviews.

trevnorris reviewed Jun 4, 2015
View reviewed changes

mscdex added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 4, 2015

ofrobots reviewed Jun 4, 2015
View reviewed changes

trevnorris force-pushed the pass-provider-and-cb branch from 0a87ffd to e0268e3Compare June 4, 2015 17:42

trevnorris reviewed Jun 4, 2015
View reviewed changes

trevnorris force-pushed the pass-provider-and-cb branch 6 times, most recently from 67b3114 to 6965730Compare June 8, 2015 23:00

trevnorris force-pushed the pass-provider-and-cb branch 2 times, most recently from 9c78909 to c47c024Compare June 9, 2015 22:50

This was referenced Jun 10, 2015
test: test-cluster-worker-wait-server-close.js is hanging #1933
Closed
cluster: fix broken/hanging tests #1934
Closed

trevnorris force-pushed the pass-provider-and-cb branch 3 times, most recently from 0890f6c to a511c2eCompare June 10, 2015 23:14

trevnorris force-pushed the pass-provider-and-cb branch from a511c2e to 49d168bCompare June 11, 2015 02:29

bnoordhuis reviewed Jun 14, 2015
View reviewed changes

Comment threadsrc/async-wrap.cc Outdated
bnoordhuisJun 14, 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.
Magic constant.

trevnorris force-pushed the pass-provider-and-cb branch from 49d168b to bb669c4Compare June 15, 2015 21:40

trevnorris force-pushed the pass-provider-and-cb branch from bb669c4 to 5e792aaCompare June 17, 2015 18:07

trevnorris closed this Jun 17, 2015

trevnorris deleted the pass-provider-and-cb branch June 17, 2015 19:04

rvagg mentioned this pull request Jun 18, 2015
Release proposal: v2.3.1 #1996
Closed

Uh oh!

Uh oh!

Conversation

trevnorris commented Jun 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jun 6, 2015

Uh oh!

trevnorris commented Jun 8, 2015

Uh oh!

bnoordhuis commented Jun 8, 2015

Uh oh!

trevnorris commented Jun 8, 2015

Uh oh!

trevnorris commented Jun 9, 2015

Uh oh!

trevnorris commented Jun 10, 2015

Uh oh!

jbergstroem commented Jun 10, 2015

Uh oh!

trevnorris commented Jun 10, 2015

Uh oh!

Olegas commented Jun 10, 2015

Uh oh!

trevnorris commented Jun 10, 2015

Uh oh!

trevnorris commented Jun 11, 2015

Uh oh!

trevnorris commented Jun 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Jun 15, 2015

Uh oh!

trevnorris commented Jun 17, 2015

Uh oh!

bnoordhuis commented Jun 17, 2015

Uh oh!

trevnorris commented Jun 17, 2015

Uh oh!

trevnorris commented Jun 17, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants