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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Haven't read the test yet. Logic looks sound.



================
Comment at: llvm/include/llvm/XRay/Profile.h:38
+/// |Filename|, this function will return an Error condition appropriately.
+Expected<Profile> loadProfile(StringRef Filename);
+
----------------
Do we have a better way of passing files around as streams of bytes or a DataExtractor so that we can more easily work with sockets or other data streams later?


================
Comment at: llvm/lib/XRay/Profile.cpp:69-72
+    if (CurrentOffset == Offset)
+      return make_error<StringError>(
+          Twine("Error parsing path at offset '") + Twine(CurrentOffset) + "'",
+          std::make_error_code(std::errc::invalid_argument));
----------------
This pattern is repeated several times. Set the current offset, read something, check to see that the offset moved, and update the current offset. Did you think about abstracting to avoid mistakes like forgetting to update CurrentOffset?


================
Comment at: llvm/lib/XRay/Profile.cpp:130
+
+  auto RevPath = reverse(P);
+
----------------
Since this function is pretty long, it might help to call this RootToLeafPath.


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


================
Comment at: llvm/lib/XRay/Profile.cpp:194-196
+    auto Res = ThreadProfileIndex.insert(
+        {Block.Thread, PathDataMapPtr{new PathDataMap()}});
+    auto &It = Res.first;
----------------
It might be worth noting here that you intentionally disregard whether the insert added a new element.


================
Comment at: llvm/lib/XRay/Profile.cpp:204-208
+      if (!Inserted) {
+        auto &ExistingData = PathDataIt->second;
+        ExistingData.CallCount += Data.CallCount;
+        ExistingData.CumulativeLocalTime += Data.CumulativeLocalTime;
+      }
----------------
Since this is the only difference between the two loops, it could make the code more simple at a small performance cost (boolean check) to simply run the same loop twice even though Inserted will always be true the first time.


================
Comment at: llvm/lib/XRay/Profile.cpp:222
+
+Profile mergeProfilesByStack(const Profile &L, const Profile &R) {
+  Profile Merged;
----------------
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.


================
Comment at: llvm/lib/XRay/Profile.cpp:376
+
+  // Once we've gone through the Trace, we now create on Block per thread in the
+  // Profile.
----------------
on Block per thread -> one Block per thread


https://reviews.llvm.org/D48370