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

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

> (There are visible changes around the name associated with the tracer in lld that someone familiar with this in lld should approve.)

I think they're pretty safe. But yeah, I will ping the other LLD people about that once we've settled on the core ThreadPool changes.

> @int3, apologies if my suggestion of adding into ThreadPool has made this more complicated! Your original change may be okay as a quicker fix, moving to ThreadPool could be a follow up.

No worries & no rush; I'm happy to hash this out :) Was just a bit busy for the last couple of days.



================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118550