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

Rewrite to use actual async_hooks #17

Closed
Fishrock123 opened this issue Sep 18, 2016 · 10 comments
Closed

Rewrite to use actual async_hooks #17

Fishrock123 opened this issue Sep 18, 2016 · 10 comments
Assignees

Comments

@Fishrock123
Copy link
Collaborator

See nodejs/node#8531

I've already started on this and got it working, but there are some relational challenges with timers and some underlying resources such as the HTTP parser.

I'll probably need to fix #16 in the process and use a flat data file by UID, and convert some of the render ordering to use that. (To deal with re-used timers...)

@Fishrock123 Fishrock123 self-assigned this Sep 18, 2016
@AndreasMadsen
Copy link
Owner

AndreasMadsen commented Sep 18, 2016

Yes!, was thinking about this too. I have previously discovered a few bugs in async_wrap with dprof. It would be great to have it working with async_hooks, we might discover a few issues there too.

I'll probably need to fix #16 in the process and use a flat data file by UID, and convert some of the render ordering to use that.

Not quite sure how #16 is related to this. It sounds like you want to create a graph rather than a tree.

@Fishrock123
Copy link
Collaborator Author

Fishrock123 commented Sep 18, 2016

@AndreasMadsen sorry, my knowledge of data structures is not the best.

Let's put it this way:

Handles with the same ID should display on the same line in the visualizer even if they are triggered from different points.

Now, that may not make 100% sense at the current time but I'm not sure how else to display timers is a conceptual and technically correct way.

That is, if you take the same timer object, do an insert of it (active), unenroll it (remove it), and re-insert it it, it should probably keep the same ID while firing init -> destroy -> init.

@AndreasMadsen
Copy link
Owner

AndreasMadsen commented Sep 18, 2016

That is, if you take the same timer object, do an insert of it (active), unenroll it (remove it), and re-insert it it, it should probably keep the same ID while firing init -> destroy -> init.

Ah, okay. That should be fine. Though the idea of uids popping back into existence is a little confusion and perhaps dangerous.

@Fishrock123
Copy link
Collaborator Author

Fishrock123 commented Sep 18, 2016

Take, for example, this.

The handles with the line in-between are actually the same, the timeout was created and the updated three times, from different places, causing re-insertion.

timers-async_hooks

UIDs at the current time (with patches for timers): 12, 17, 20, then 26.

@AndreasMadsen
Copy link
Owner

AndreasMadsen commented Sep 18, 2016

I think my understanding of the node.js timer implementation is vastly insufficient to understand how a timer can be updated without becoming a different timer. In normal JS I would write createTimeout(timer); timer = setTimeout(fn, update), but obviously that is not what is going on here.

In any case it does complicate things quite a bit, because a single handle will have multiple parents (no longer a tree). However because there is still a parent-child relationship, it should be possible to visualize it like it's done now, see Topological Sorting of a DAG.

@Fishrock123
Copy link
Collaborator Author

Fishrock123 commented Sep 18, 2016

I think my understanding of the node.js timer implementation is vastly insufficient to understand how a timer can be updated without becoming a different timer.

I suppose this depends on the definition of "timer".

Timers are not restricted to being Timeouts and can actually be any any object with an ._onTimeout = function(){} property that is then passed to timers.enroll() and timers.active().

That being said, in any timers implementation, changing the timeout duration or timeout start means re-ordering the timer in an underlying queue.

In our case, due to optimizations, changing the duration may cause a new underlying TimerWrap to be created. Changing the timeout start will not.

... etc

Perhaps it really does only make sense to fire init/destroy once for these. Not 100% sure yet.

@Fishrock123
Copy link
Collaborator Author

Fishrock123 commented Sep 18, 2016

Timers are not restricted to being Timeouts and can actually be any any object with an ._onTimeout = function(){} property that is then passed to timers.enroll() and timers.active().

Thus, adding to the confusion, the timer in the above picture is really a Net.Socket.

@Fishrock123
Copy link
Collaborator Author

Maybe just using the same UID without firing events for re-insertion before destroy() is the way to go:

timers-maybe-better-async_wrap

(handle is orange and UID 12.)

Refs: nodejs/node#8531 (comment)

@Fishrock123
Copy link
Collaborator Author

#19

@AndreasMadsen
Copy link
Owner

Thanks for the work, I've landed async_hooks support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants