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

[REQ] Resource Binding #104

Closed
poor1017 opened this issue Mar 27, 2023 · 9 comments
Closed

[REQ] Resource Binding #104

poor1017 opened this issue Mar 27, 2023 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@poor1017
Copy link

Describe the new feature

Can we support binding a resource to a thread?

Code example

BS::thread_pool pool(12);
for (size_t i = 0; i < 12; ++i) {
    bool ret = pool.attach_resource(int thread_id, Resource *rsrc);
}
@poor1017 poor1017 added the enhancement New feature or request label Mar 27, 2023
@v1993
Copy link

v1993 commented Apr 7, 2023

I think a better solution would be to pass a function that takes thread id as a parameter to thread_pool constructor, then store relevant resources in thread_local variables. Such constructor will block until all thread initialization calls are complete to make addressing locals in caller safe. So your example becomes

constexpr auto thread_count = 12;
std::array<Resource, thread_count> resource_array;
thread_local Resource* thread_resource;
...
BS::thread_pool pool{thread_count, [&resource_array](int thread_id) {
	thread_resource = &resource_array[thread_id];
}};

In this case locking is not needed, but you can make array local to caller function (which is likely a good idea) and move its members to thread local variable instead of storing a pointer. Or initialize resources directly in threads instead of caller, which makes sense for a lot of resource types.

A major reason I'd like to see this approach implemented instead of resource binding is because it allows to set up OpenGL context for every thread in pool, something that specifically requires running initialization code once per thread. Merely binding resource to thread would not allow this (well, you can make a thread local flag and check it at the beginning of every task, but just doing initialization properly is a much better idea). Plus this should be easier to implement than resource binding approach.

@poor1017
Copy link
Author

poor1017 commented Apr 7, 2023

@v1993

That's a good approach. I will try.

Thank you!

@v1993
Copy link

v1993 commented Apr 7, 2023

@poor1017 You may want to take a look at PR #105, I've implemented initialization function accepting thread id in pool as an optional argument to constructor and reset method, like outlined in example I've provided above.

@poor1017
Copy link
Author

poor1017 commented Apr 7, 2023

Got it

@bshoshany
Copy link
Owner

Thanks for opening this issue, @poor1017! However, I'm not sure why this is needed. Let's continue the discussion in #105 - hopefully one of you can provide a more detailed explanation and some concrete code examples. Meanwhile I am closing this issue, as PR #105 is already addressing it.

@poor1017
Copy link
Author

@v1993 According to your approach, the variable thread_local should belong to each thread. So, where do I create this variable? Is it in the constructor of thread_pool? And, once I submit a task, where do I get this variable from during any execution?

@poor1017
Copy link
Author

@v1993 Or we can create a map from std::this_thread::get_id() to thread_id in thread_init_function?

@bshoshany
Copy link
Owner

@poor1017 you should wait for v4.0.0, to be released soon, which will implement thread initialization functions and will show some examples of how to use them. It will also have a function to get the index of the current thread, so there's no need for a map.

@poor1017
Copy link
Author

nice!

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

No branches or pull requests

3 participants