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

Provide a publicly-accessible Core ID #149

Merged
merged 1 commit into from Jan 18, 2017
Merged

Provide a publicly-accessible Core ID #149

merged 1 commit into from Jan 18, 2017

Conversation

vorner
Copy link
Contributor

@vorner vorner commented Jan 15, 2017

Allow distinguishing between multiple Cores and associating data with
them through a HashMap or similar.

This is a proposal for #147.

@alexcrichton
Copy link
Contributor

Could the Ord and PartialOrd implementations get dropped for now? I wouldn't want anything accidentally relying on an unintentional ordering.

I think it may also be worth documenting that ids are never recycled. I doubt anyone's going to create more than 2^32 event loops in one process...

Allow distinguishing between multiple `Core`s and associating data with
them through a `HashMap` or similar.
@vorner
Copy link
Contributor Author

vorner commented Jan 17, 2017

I don't mind dropping them, I was thinking someone might prefer using Map instead of HashMap, but I don't see a good reason for anyone wanting to do that.

I tried to add that documentation. Though, I'm not 100% sure someone won't try creating so many event loops, people do all kinds of crazy stuff, and someone might be creating a short-lived event loops and then drooping them for some reason. But that's certainly out of the scope of this issue.

(I hope you don't mind me just force-pushing an amended commit ‒ usually I stack fixup commits and squash them before merge to make reviewing the new changes easier, but this seems like a very small thing to bother)

@tailhook
Copy link
Contributor

Well, technically I'd say 2^32 loops easily possible.

Consider some curl-like library creates a loop on curl::fetch(), just because providing a synchronous API on top of async is a very sensible thing. And you can have a long running crawler that does fetches in multiple threads. You know, making just a 1000 requests per second in such a system will overflow a counter in 49 days :)

@vorner
Copy link
Contributor Author

vorner commented Jan 18, 2017

@tailhook We could change the usize to u64, but u64 atomic is still unstable (rust-lang/rust#32976), so it would need to be under a mutex currently (which probably isn't that bad, creating new loops wouldn't be performance-sensitive operation).

But as this limitation isn't brought in by this PR, should we create a separate issue for that?

@alexcrichton
Copy link
Contributor

@tailhook heh true! If we stick with the opaque representation as-is though then we're at least flexible to add more bits in the future :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants