[PATCH] D118550: [Support] Have ThreadPool initialize a TimeTraceProfiler per thread

[Jez Ng via Phabricator via llvm-commits]

int3 planned changes to this revision.
int3 added inline comments.


================
Comment at: llvm/lib/Support/ThreadPool.cpp:37
   int newThreadCount = std::min<int>(requested, MaxThreadCount);
+  TimeTraceProfiler *MainThreadProfiler = getTimeTraceProfilerInstance();
   while (static_cast<int>(Threads.size()) < newThreadCount) {
----------------
mehdi_amini wrote:
> russell.gallop wrote:
> > int3 wrote:
> > > int3 wrote:
> > > > mehdi_amini wrote:
> > > > > russell.gallop wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > It seems that we'll always use the instance initialized in the thread that calls "grow".  Also, this instance has to be setup before the call to grow, and the thread can't reinitialize it for the lifetime duration of the ThreadPool if I understand correctly.
> > > > > > > 
> > > > > > > I'm not sure this makes sense in the full generality of the ThreadPool API?
> > > > > > > I'm not sure this makes sense in the full generality of the ThreadPool API?
> > > > > > 
> > > > > > Do we agree that time tracing all threads used by the ThreadPool is desirable and worth pursuing?
> > > > > This could be a useful feature to have a tracing feature for the ThreadPool. I'm not sure about:
> > > > > - The expected behavior in terms of threading (with respect to the thread pool creation, the growing of the pool, or the enqueuing of a task).
> > > > > - The current `TimeTraceProfiler` which is non-trivially coupled to some global state, making this all harder to reason about.
> > > > > It seems that we'll always use the instance initialized in the thread that calls "grow".
> > > > 
> > > > Actually we are just copying the value of its TimeTraceGranularity member and using that to initialize a new thread-local instance. I could have just copied the TimeTraceGranularity value itself, but I figured this was a slightly nicer abstraction -- if in the future we add more fields to TimeTraceProfilerInstance, we can keep the same initialization method signature.
> > > > 
> > > > You are right that this makes things a little less general though. In particular, there is no way to have different granularities per profiler instance -- every thread must use the same value. IDK if that's an issue... after all, the places where the TimeProfiler is getting used ATM don't take advantage of this flexibility.
> > > > 
> > > > > the thread can't reinitialize it for the lifetime duration of the ThreadPool if I understand correctly
> > > > 
> > > > I don't think there's a use case for reinitializing it...
> > > > 
> > > > That said I didn't write the TimeProfiler, so maybe @russell.gallop can confirm.
> > > bump -- @russell.gallop, could you chime in here?
> > > You are right that this makes things a little less general though. In particular, there is no way to have different granularities per profiler instance -- every thread must use the same value. IDK if that's an issue... after all, the places where the TimeProfiler is getting used ATM don't take advantage of this flexibility.
> > 
> > Yes, I don't imagine it is very useful to have different granularities.
> > 
> > > I don't think there's a use case for reinitializing it...
> > > That said I didn't write the TimeProfiler, so maybe @russell.gallop can confirm.
> > 
> > I'm not sure I follow the use of the Thread Pool where this would be required...
> > 
> > I think @anton-afanasyev added the time profiler, I extended for LLD tracing, but only really with the ThinLTO threading case in mind. I imagine that the profiler could be re-engineered to meet what the thread pool can do, but I'm not really aware of what the "full generality of the thread pool" is so can't really say more than that.
> > 
> > I don't think I'll have time to do this myself. Let me know if you don't have time and I can ask if there is someone around here who can take a look at this.
> The ThreadPool isn't a "global" thing, but the profiler is. It seems to me that there is a mismatch in terms of lifetime / lifecycle that creeps up here and does not make it a natural fit for the ThreadPool to know directly about the profiler.
> 
> For example a MLIRContext (similar to LLVMContext) owns a ThreadPool, you may through the lifetime of a process create and destroy multiple ThreadPool.
> 
> The thread which creates the ThreadPool isn't necessarily the one that will enqueue work to the pool. In MLIR for example, the pool will get work enqueued when a pass manager is executed: what does it mean in terms of profiler instance?
> 
> The kind of things that can be surprising with the implementation in this patch is that a sequence like:
> - Create a ThreadPool
> - Setup profiler
> - Grow the pool to more thread
> - schedule
> 
> would lead to only some threads having the profiler enabled (the ones that grew post setup).
> More simpler "gotcha" is:
> - Create a ThreadPool (indirectly, from the user point of it is "initialize the compiler" or "Create a MLIRContext")
> - Setup profiler
> - run
> 
> this wouldn't get any thread other that the current one having profiler setup.
> However many construct like parallel_for will schedule iterations in the pool but also use the current thread to run work, so work executed in the current thread would get different behavior from the work setup in the pool.
> Variant of this behavior can include:
> 
> - Setup profiler
> - Create a ThreadPool (indirectly, from the user point of it is "initialize the compiler" or "Create a MLIRContext")
> - `parallel_for` from another thread
> 
> This time the parallel iteration in the thread pool would have the profiler setup, but not the ones directly executed in the calling thread.
> 
> 
got it, I see why you are uncomfortable with the change now. I will revert to the earlier non-ThreadPool-invasive version of this diff. Thanks for the insight!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118550/new/

https://reviews.llvm.org/D118550