[PATCH] D133083: [support] Allow TimeProfiler tracing across threads

[Mark Shields via Phabricator via llvm-commits]

mbs-modular added a comment.

Thanks @russell.gallop.

> Does this help with the case @mehdi_amini outlined here: https://reviews.llvm.org/D118550/new/#3323665 ?

If I'm reading correctly that is lamenting the need for each thread to establish and tear down it's own context as a consequence of relying on the TLS profiling context. That's not a concern for us, and the TLS approach seems inevitable given the current push/pop API. A global lock-free ring for all events is probably a better approach, but that's a new impl entirely I think.

Our use case is pretty simple:

  // thread A
  auto entry = timeTraceProfilerBeginEntry("e")
  enqueuWorkItem([entry = std::move(entry)]() {
      // thread B
      timeTraceProfilerEndEntry(std::move(entry));
      ... do the work ...
  })

The entries for "e" now measure scheduling latency.

> Would it be possible to split those out to to a separate NFC patch

Sure thing.

> I'm also trying to think whether the change to high_resolution_clock might have side-effects

Likewise a bit troubled by that. I think we should pessimistically assume there's a memory barrier in the high res clock, so must make sure to never ask for now() from it if tracing is disabled. I think I need an extra if (TimeTraceProfilerInstance) test in timeTraceProfilerBeginEntry, and will check elsewhere.

> there is an outstanding bug on time-trace (https://github.com/llvm/llvm-project/issues/56554).

Yes, appears unrelated. I'm avoiding any caller changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133083