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

Refactor Waker to make unsafe parts clearer and use less unsafe #13

Closed
wants to merge 5 commits into from

Conversation

eholk
Copy link
Collaborator

@eholk eholk commented Apr 1, 2022

The main change here is to make Internals.shared an Arc<Shared> instead of a *const Shared. This lets us avoid some unsafe casts and manually dropping.

It also splits the functions in the vtable into an unsafe wrapper that casts the this argument and then calls a safe function that does the actual work. This is to make it clearer which steps are actually unsafe and which are safe.

@udoprog
Copy link
Owner

udoprog commented Apr 1, 2022

This has the (maybe unintended) side effect of requiring the Arc<Shared> to be cloned and the clone dropped every time we call poll_with_ref. It's nice to avoid this since avoiding it makes polling more efficient. And less prone to having to contend with cache coherence slowdowns due to the Arc<Shared> being shared across cores*. All though I don't know if we have any benches covering this.

I think the design here was inspired by an earlier variant of Tokio's internal poll impl (But using Arc instead of custom ref-counting). There might be some improvements worth incorporating from there.

@udoprog udoprog added the enhancement New feature or request label Apr 1, 2022
@eholk
Copy link
Collaborator Author

eholk commented Apr 1, 2022

Ah, that's a good point. Yeah, that sounds like the kind of thing that could become a performance issue. It's probably worth adding a comment explaining why.

eholk added 3 commits April 1, 2022 17:51
Using an Arc instead prevents an allocation on every clone of the waker.
Instead, we only need the allocation when converting from a RefWaker
to Internals. We still end up with the same number of atomic ref count
changes since before we had to increment the count on the Arc<Shared>
but that is no longer necessary.
@eholk
Copy link
Collaborator Author

eholk commented Apr 2, 2022

I pushed a few more commits that I think improve things overall here.

First is I split Internals into Internals and RefWaker. We sort of had two behaviors in the same struct before, one that referred to an &Arc<Shared> and one that referred to Arc<Shared>. Now these behaviors are separated into different types. When the RefWaker is cloned, it converts into an Internals waker` that is heap-allocated. The downside is that it duplicates a lot of the code in the waker vtables, but hopefully it's clearer now how the various casts and destructors interact.

Second, now that we can tell from the types whether we are heap-allocated, we can make it so Internals is always wrapped in an Arc rather than a Box. This means clone just needs to bump the ref count rather than doing a new allocation. Since before we had to increment the ref count on the Arc<Shared> and that is no longer needed, we have exactly the same number of ref count operations but fewer heap allocations, so that seems like a win.

This makes it so that each waker has at most one heap allocation (the one where it promotes from stack allocated to heap allocated), rather than a potentially unbounded number. This means this change might make solving #14 unnecessary.

@eholk eholk mentioned this pull request Apr 9, 2022
@eholk
Copy link
Collaborator Author

eholk commented Apr 18, 2022

Closing this one since it was included in #15.

@eholk eholk closed this Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants