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

[Russell Gallop via Phabricator via llvm-commits]

russell.gallop added a comment.

Thanks for splitting this out. Generally looks good with some comments, and thanks for adding the unit test.



================
Comment at: llvm/include/llvm/Support/TimeProfiler.h:53
+//
+// Sub-threads should be wrapped in timeTraceProfilerInitialize and
+// timeTraceProfilerFinishThread calls. The main process should
----------------
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.


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


================
Comment at: llvm/unittests/Support/TimeProfilerTest.cpp:1
+//===- unittests/TimerTest.cpp - Timer tests ------------------------------===//
+//
----------------
Stale comment.


================
Comment at: llvm/unittests/Support/TimeProfilerTest.cpp:8
+//===----------------------------------------------------------------------===//
+// These are bare-minimum 'smake' tests of the time profiler. Not tested:
+//  - multi-threading
----------------
smoke tests?


================
Comment at: llvm/unittests/Support/TimeProfilerTest.cpp:36
+TEST(TimeProfiler, Scope_Smoke) {
+  setup();
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133153