[PATCH] D34863: [XRay][tools] Function call stack based analysis tooling for XRay traces

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Tests?



================
Comment at: tools/llvm-xray/xray-stacks.cc:197
+    auto Node = &*I;
+    if (Parent == nullptr) {
+      Roots.emplace_back(Node);
----------------
dblaikie wrote:
> Skip {} on single line blocks
I'd probably write this as "(!Parent)" but I'm not sure if there's an especially prevailing convention in the LLVM codebase.


================
Comment at: tools/llvm-xray/xray-stacks.cc:197-199
+    if (Parent == nullptr) {
+      Roots.emplace_back(Node);
+    }
----------------
Skip {} on single line blocks


================
Comment at: tools/llvm-xray/xray-stacks.cc:198
+    if (Parent == nullptr) {
+      Roots.emplace_back(Node);
+    }
----------------
Prefer push_back over emplace_back


================
Comment at: tools/llvm-xray/xray-stacks.cc:205
+  TrieNode *findRoot(int32_t FuncId) {
+    for (auto Node : Roots)
+      if (Node->FuncId == FuncId)
----------------
use 'auto *' in the range-for to indicate that this is a pointer


================
Comment at: tools/llvm-xray/xray-stacks.cc:205-208
+    for (auto Node : Roots)
+      if (Node->FuncId == FuncId)
+        return Node;
+    return nullptr;
----------------
dblaikie wrote:
> use 'auto *' in the range-for to indicate that this is a pointer
Alternatively, consider find_if (the llvm:: variant that takes a range rather than begin/end)


================
Comment at: tools/llvm-xray/xray-stacks.cc:221-225
+        if (auto Root = findRoot(R.FuncId)) {
+          TS.emplace_back(Root, R.TSC);
+        } else {
+          TS.emplace_back(createTrieNode(R.FuncId, nullptr), R.TSC);
+        }
----------------
Usually omit {} on single line blocks


================
Comment at: tools/llvm-xray/xray-stacks.cc:221-225
+        if (auto Root = findRoot(R.FuncId)) {
+          TS.emplace_back(Root, R.TSC);
+        } else {
+          TS.emplace_back(createTrieNode(R.FuncId, nullptr), R.TSC);
+        }
----------------
dblaikie wrote:
> Usually omit {} on single line blocks
Alternatively, consider a conditional operator:

  TS.emplace_back(Root ? Root : createTrieNode(R.FuncId, nullptr), R.TSC);


================
Comment at: tools/llvm-xray/xray-stacks.cc:330-347
+          {
+            auto E = std::make_pair(Top, TopSum);
+            TopStacksBySum.insert(std::lower_bound(TopStacksBySum.begin(),
+                                                   TopStacksBySum.end(), E,
+                                                   greater_second),
+                                  E);
+            if (TopStacksBySum.size() == 11)
----------------
Not sure these explicit scopes ("{}") are sufficiently valuable (reducing the scope of 'E') to worry about/include?


================
Comment at: tools/llvm-xray/xray-stacks.cc:352
+        for (const auto *C : Top->Callees)
+          S.emplace_back(C);
+      }
----------------
Prefer push_back when emplace_back and puhs_back both do the same thing.

(for the same reason that one should prefer copy init (T u = v) over direct init (T u(v)) - because copy init can only cause implicit conversions, whereas direct init can perform explicit conversions - so it's easier to read code that uses the less powerful construct, since it can't do "interesting" things)


================
Comment at: tools/llvm-xray/xray-stacks.cc:374
+
+static CommandRegistration Unused(&Stack, []() -> Error {
+  // Load each file provided as a command-line argument. For each one of them
----------------
Could potentially write this as "[] {" I think, not sure if that's more readable though.

("()" can be omitted in a lambda, as can the return type if it's deducible from the return expressions consistently - at least I think that's supported on LLVM's supported compilers)


================
Comment at: tools/llvm-xray/xray-stacks.cc:410-411
+    auto &T = *TraceOrErr;
+    DenseMap<int32_t, SmallVector<std::pair<int32_t, uint64_t>, 8>>
+        ThreadStacks;
+    for (const auto &Record : T) {
----------------
Unused variable?


https://reviews.llvm.org/D34863