[PATCH] D48370: [XRay][llvm] Load XRay Profiles

[Keith via Phabricator via llvm-commits]

kpw accepted this revision.
kpw added a comment.
This revision is now accepted and ready to land.

Please consider my comments about the tests before pushing.



================
Comment at: llvm/lib/XRay/Profile.cpp:187-188
+    for (const auto &PathAndData : Block.PathData) {
+      auto &PathID = PathAndData.first;
+      auto &Data = PathAndData.second;
+      auto NewPathID = Merged.internPath(cantFail(L.expandPath(PathID)));
----------------
dberris wrote:
> kpw wrote:
> > There is a bit of the cascading auto here where the compiler is fine, but a reader might get a bit lost.
> > 
> > It is not too verbose and might help to make these:
> > Profile::PathID &PathID and
> > Profile::Data &Data.
> It actually turns out to be more cumbersome, because of the repetition of the type and the identifier being spelled the same:
> 
> ```
> const Profile::PathID &PathID = ...;
> const Profile::Data &Data = ...;
> ```
In the words of the dude: "That's just like... your opinion man."


================
Comment at: llvm/lib/XRay/Profile.cpp:222
+
+Profile mergeProfilesByStack(const Profile &L, const Profile &R) {
+  Profile Merged;
----------------
dberris wrote:
> kpw wrote:
> > Another idea for code re-use. mergeProfilesByStack and mergeProfilesByThread could both call a routine like mergeProfilesByThread but with an additional parameter that is a callable that picks out the thread id from a function. It would simply return zero for mergeProfilesByStack. The code could be identical and create a map with a single element.
> > 
> > If you prefer to specialize that's your call, but this is less performance critical IIUC since it's not in compiler-rt.
> The algorithms are very different, since the additional data structure for grouping by threads is by definition unnecessary in the merging across stacks losing information about threads.
That's fair.


================
Comment at: llvm/unittests/XRay/ProfileTest.cpp:49-56
+  EXPECT_TRUE(errorToBool(P.addBlock({})));
+  EXPECT_TRUE(errorToBool(P.addBlock({1, {}})));
+  EXPECT_FALSE(errorToBool(P.addBlock(
+      Profile::Block{Profile::ThreadID{1},
+                     {
+                         {P.internPath({2, 1}), Profile::Data{1, 1000}},
+                         {P.internPath({3, 2, 1}), Profile::Data{10, 100}},
----------------
Could you add a comment explaining each assertion. If these fail, the aggregate initializers require a lot from an unfamiliar reader.

Suggestions:

// Empty struct should fail
...
// Thread ID with no data should fail
...
// Thread ID with data should succeed

Die hard style reviewers would argue that this should be three tests! ;P 


================
Comment at: llvm/unittests/XRay/ProfileTest.cpp:60-72
+  Profile P0, P1;
+  EXPECT_FALSE(errorToBool(P0.addBlock(
+      Profile::Block{Profile::ThreadID{1},
+                     {{P0.internPath({2, 1}), Profile::Data{1, 1000}}}})));
+  EXPECT_FALSE(errorToBool(P1.addBlock(
+      Profile::Block{Profile::ThreadID{1},
+                     {{P1.internPath({2, 1}), Profile::Data{1, 1000}}}})));
----------------
Maybe choose some values that are different for the different stacks so that if the test will fail if the merge combines an incorrect permutation pairing up the stacks?

Maybe also add a unique stack to one or both profiles to check that the implementation is robust to stacks that don't have a match.


================
Comment at: llvm/unittests/XRay/ProfileTest.cpp:101
+  EXPECT_FALSE(errorToBool(P1.addBlock(
+      Profile::Block{Profile::ThreadID{1},
+                     {{P1.internPath({2, 1}), Profile::Data{1, 1000}}}})));
----------------
Use a different thread ID here to verify we're not matching on threads?


================
Comment at: llvm/unittests/XRay/ProfileTest.cpp:118
+
+TEST(ProfileTest, MergeProfilesByStackAccumulate) {
+  std::vector<Profile> Profiles(3);
----------------
Should there be MergeProfilesByThreadAccumulate as well?


================
Comment at: llvm/unittests/XRay/ProfileTest.cpp:121-127
+      Profile::ThreadID{1},
+      {{Profiles[0].internPath({2, 1}), Profile::Data{1, 1000}}}})));
+  EXPECT_FALSE(errorToBool(Profiles[1].addBlock(Profile::Block{
+      Profile::ThreadID{1},
+      {{Profiles[1].internPath({2, 1}), Profile::Data{1, 1000}}}})));
+  EXPECT_FALSE(errorToBool(Profiles[2].addBlock(Profile::Block{
+      Profile::ThreadID{1},
----------------
Use different thread IDs?


https://reviews.llvm.org/D48370