[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -227,7 +265,7 @@ class ThreadPool {
class ThreadPoolTaskGroup {
GeorgeARM wrote:
Thanks for explaining @joker-eph. Looks good to me.
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM approved this pull request. https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -227,7 +265,7 @@ class ThreadPool {
class ThreadPoolTaskGroup {
GeorgeARM wrote:
So the way I see this is:
I have a public API that looks like:
```
virtual void asyncEnqueue(std::function Task, ThreadPoolTaskGroup
*Group) = 0;
// or similar
```
My thoughts:
* `ThreadPoolTaskGroup` implementation is actually internal to the `ThreadPool`
* `ThreadPoolTaskGroup` being passed to the API but only used internally as
just a UID. No member functions being called internally. Moreover, the object
definition itself is a RAII wrapper on top of the ThreadPool API implementation
so I find that this is an unnecessary circular dependency and coupling.
* Uniqueness is `guaranteed` through memory addressing. Someone can `break`
uniqueness when going through the public `ThreadPool` API.
* Type safety; sure you don't have implicit conversions going on.
What `guarantees` to you uniqueness is essentially when you use RAII wrapper
itself and the fact that calls at termination: ` ~ThreadPoolTaskGroup() {
wait(); }` which will go through; process the tasks and erase the group from
the `ActiveGroups`.
So what I think is that passing instead of passing `*this` you can convert the
address to a string/int and pass this instead.
Eitherway, please ignore my comment if it doesn't make sense; as said not
familiar with the codebase. I just find strange a bit this aspect of the API.
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -227,7 +265,7 @@ class ThreadPool {
class ThreadPoolTaskGroup {
GeorgeARM wrote:
Sure you can convert the address of the object to a string or int that is fine;
it is not the best but not worse than what is there. You can do this inside the
`ThreadPoolTaskGroup` which is essentially a RAII wrapper.
What you get is probably a cleaner API IMHO and don't impose to the
implementations to have to consume a struct that by definition loops around the
interface.
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -227,7 +265,7 @@ class ThreadPool {
class ThreadPoolTaskGroup {
GeorgeARM wrote:
Btw, I still think that passing a `ThreadPoolTaskGroup` for the sole purpose of
a uid is a bit brittle as you bind the interface with this construct but
internally to the `ThreadPool` no member function of it is called?!
We could change for example this with a `std::string` with probably minimal
changes to any call site.
Apologies if that is not right but I am not familiar with the code.
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -92,30 +104,20 @@ class ThreadPool {
);
}
- /// Blocking wait for all the threads to complete and the queue to be empty.
- /// It is an error to try to add new tasks while blocking on this call.
- /// Calling wait() from a task would deadlock waiting for itself.
- void wait();
-
- /// Blocking wait for only all the threads in the given group to complete.
- /// It is possible to wait even inside a task, but waiting (directly or
- /// indirectly) on itself will deadlock. If called from a task running on a
- /// worker thread, the call may process pending tasks while waiting in order
- /// not to waste the thread.
- void wait(ThreadPoolTaskGroup );
-
- // Returns the maximum number of worker threads in the pool, not the current
- // number of threads!
- unsigned getMaxConcurrency() const { return MaxThreadCount; }
-
- // TODO: misleading legacy name warning!
- LLVM_DEPRECATED("Use getMaxConcurrency instead", "getMaxConcurrency")
- unsigned getThreadCount() const { return MaxThreadCount; }
+private:
+ /// Asynchronous submission of a task to the pool. The returned future can be
+ /// used to wait for the task to finish and is *non-blocking* on destruction.
+ template
+ std::shared_future asyncImpl(std::function Task,
+ ThreadPoolTaskGroup *Group) {
GeorgeARM wrote:
Ignore I just saw the code above.
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -227,7 +265,7 @@ class ThreadPool {
class ThreadPoolTaskGroup {
GeorgeARM wrote:
Get the point of @aganea; the `ThreadPoolTaskGroup` is a bit tricky to reason
when it comes to customized extensibility as the concrete implementation is
injected to the interface itself. IMHO I think that this construct can sit on
top of the `ThreadPoolInterface` itself (as it kind of does now) without
semantically injecting itself in the pool. Someone can implement this through
some API that returns a trackable construct; e.g. a `TaskID` or some kind of
`Future` interface from the `asyncEnqueue`. Not sure if that's worth the effort
though.
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
@@ -92,30 +104,20 @@ class ThreadPool {
);
}
- /// Blocking wait for all the threads to complete and the queue to be empty.
- /// It is an error to try to add new tasks while blocking on this call.
- /// Calling wait() from a task would deadlock waiting for itself.
- void wait();
-
- /// Blocking wait for only all the threads in the given group to complete.
- /// It is possible to wait even inside a task, but waiting (directly or
- /// indirectly) on itself will deadlock. If called from a task running on a
- /// worker thread, the call may process pending tasks while waiting in order
- /// not to waste the thread.
- void wait(ThreadPoolTaskGroup );
-
- // Returns the maximum number of worker threads in the pool, not the current
- // number of threads!
- unsigned getMaxConcurrency() const { return MaxThreadCount; }
-
- // TODO: misleading legacy name warning!
- LLVM_DEPRECATED("Use getMaxConcurrency instead", "getMaxConcurrency")
- unsigned getThreadCount() const { return MaxThreadCount; }
+private:
+ /// Asynchronous submission of a task to the pool. The returned future can be
+ /// used to wait for the task to finish and is *non-blocking* on destruction.
+ template
+ std::shared_future asyncImpl(std::function Task,
+ ThreadPoolTaskGroup *Group) {
GeorgeARM wrote:
Is there a reason that this is part of the interface itself? Unless I am
misreading something? If is for reusability across interface does it make sense
to be a free function (if possible)?
https://github.com/llvm/llvm-project/pull/82094
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM commented: Sorry to pitch in; hope my comments help. Overall, looks like a nice starting drop-in interface injection on top of the what we have. Thanks @joker-eph https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [llvm] [mlir] Split the llvm::ThreadPool into an abstract base class and an implementation (PR #82094)
https://github.com/GeorgeARM edited https://github.com/llvm/llvm-project/pull/82094 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
