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

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: llvm/include/llvm/XRay/Profile.h:38
+/// |Filename|, this function will return an Error condition appropriately.
+Expected<Profile> loadProfile(StringRef Filename);
+
----------------
kpw wrote:
> 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?
Ideally we'll take a DataExtractor instance. We can add that later, at this point we don't quite need it yet.


================
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));
----------------
kpw wrote:
> 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?
Looks like this isn't something easily automated. :(

This is very much an artefact of the data extractor interface.


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


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


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


https://reviews.llvm.org/D48370