[PATCH] D133153: [support] Prepare TimeProfiler for cross-thread support

[Mark Shields via Phabricator via llvm-commits]

mbs-modular added inline comments.


================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:53
+//
+// Sub-threads should be wrapped in timeTraceProfilerInitialize and
+// timeTraceProfilerFinishThread calls. The main process should
----------------
russell.gallop wrote:
> Thanks for adding some guidance on how this is intended to be used.
> 
> I think that this could be slightly clearer/less ambiguous. timeTraceProfilerInitialize and timeTraceProfilerFinishThread should wrap the code in the thread, they shouldn't wrap thread creation.
> 
> Likewise with the main process, Initialize/Write and Cleanup are still inside the process. Wrapping the process sounds, to me, like it should be somehow outside of the process. Maybe there is a better way to phrase this, something like wrapping the code rather than the process maybe.
Done.



================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:62
+//
+// Currently, the best trace viewer is provided by the Google Perfetto
+// project, a successor of Chrome's tracing, see http://ui.perfetto.dev.
----------------
russell.gallop wrote:
> I haven't tried it but which trace viewer is best seems subjective!
> 
> Note that clang/docs/ClangCommandLineReference.rst suggests chrome://tracing or Speedscope so this could be seen to be a bit inconsistent.
Done. However no luck getting Speedscope to work (stack overflow) so I'll mention it with qualification.


================
Comment at: llvm/unittests/Support/TimeProfilerTest.cpp:36
+TEST(TimeProfiler, Scope_Smoke) {
+  setup();
+
----------------
russell.gallop wrote:
> Is there a reason not to use SetUp() and TearDown() (https://github.com/google/googletest/blob/main/docs/primer.md#writing-the-main-function)? They would then be run automatically rather than having to be in each test, making it easier to add new tests.
Ah, because in the sequel I want to test the explicit entry creation and ending is a no-op if no profiling has been set up. Added a very dummy test here to set the stage for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133153