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

[Keith via Phabricator via llvm-commits]

kpw added a comment.

Publishing existing comments. Haven't reviewed the whole patch.



================
Comment at: llvm/include/llvm/XRay/Profile.h:55-56
+
+  /// Provides a sequence of function IDs from a previously interned PathID.
+  Expected<std::vector<FuncID>> path(PathID P) const;
+
----------------
Maybe call this expand and elaborate on what type of error conditions are possible. Is the Expected just for an invalid id?


================
Comment at: llvm/include/llvm/XRay/Profile.h:59
+  /// The stack represented in |P| must be in stack order (leaf to root).
+  PathID internPath(ArrayRef<FuncID> P);
+
----------------
What happens when a caller provides an argument that has already been interned? Will they receive a new ID or the existing one?


================
Comment at: llvm/include/llvm/XRay/Profile.h:62
+  /// Appends a fully-formed Block instance into the Profile.
+  Error addBlock(Block &&B);
+
----------------
The header should document the error cases. Is it intended semantics to add a block that has a ThreadId and a PathId pair that already exist in another block? Will this accumulate the call count and cumulative time for the path? What about cases where some of the PathIds in the block have been interned and some haven't. Is there partial success?


https://reviews.llvm.org/D48370