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

Add support for std::coroutine_handle #32

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

cmazakas
Copy link
Member

@cmazakas cmazakas commented Jun 7, 2023

We need to keep with the times

https://godbolt.org/z/1andznGhq

@pdimov
Copy link
Member

pdimov commented Jun 7, 2023

What's the intended use of boost::hash<coroutine_handle>?

@cmazakas
Copy link
Member Author

cmazakas commented Jun 7, 2023

What's the intended use of boost::hash<coroutine_handle>?

In my case, it's for storing an unordered_flat_set<coroutine_handle> for use as an io_uring runtime. To this end, I can store and lookup tasks as required.

The other motivation is just a simple: "Well, the stdlib does it so we should too"

@pdimov
Copy link
Member

pdimov commented Jun 7, 2023

Aren't you going to store tasks in the unordered set, though, instead of the raw coroutine_handle? In which case you'll need to specialize boost::hash<task> anyway.

@cmazakas
Copy link
Member Author

cmazakas commented Jun 7, 2023

Aren't you going to store tasks in the unordered set, though, instead of the raw coroutine_handle? In which case you'll need to specialize boost::hash<task> anyway.

This is a good question and I don't necessarily have a great response to it.

I might wind up storing coroutine_handle<> which is type-erased on the promise_type. This way, the runtime can store arbitrary tasks so long as everything it needs is kept in the promise type or coroutine frame itself. But I'm hesitant to push for this as I have no idea at this stage if this is viable or not.

I can, instead, do some external market research and see how other people are using hash<coroutine_handle<T>>.

@pdimov
Copy link
Member

pdimov commented Oct 12, 2023

The test is overspecified; it should test that the hashes of t and t2 match and that they don't match the hash of t3 (a second task).

We don't want to guarantee that hashing a coroutine_handle produces the same result as hashing .address, because we want to be able to return the result of std::hash<std::coroutine_handle>.

If we want to be stricter than that and check that the hash isn't of extremely poor quality, we can do a collision test with something like 256 values, checking that there are no duplicates, as is done f.ex. in https://github.com/boostorg/container_hash/blob/develop/test/hash_number_test2.cpp.

@pdimov
Copy link
Member

pdimov commented Oct 17, 2023

On the other hand, for transparent hashing purposes, we do want to guarantee that boost::hash<std::coroutine_handle>()( h ) == boost::hash<void*>()( h.address() ), and if std::hash<std::coroutine_handle> is used, this won't hold. :-/

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.

2 participants