[PATCH] D31345: [XRay] [compiler-rt] Unwriting FDR mode buffers when functions are short.

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added a comment.

In https://reviews.llvm.org/D31345#710496, @kpw wrote:

> I am making some headway. My understanding is that FDR logging short circuits when the log is finalizing or finalized without returning the buffer and is currently depending on the thread exit destructor to release the buffer.


There's two parts to this -- we should be returning the buffer as soon as we realize that the log is finalizing in the implementation. On the user-side, there should be a short grace period if you want to get more of the buffer before flushing. So there's two stages to this, there's the finalizing at the FDR level and there's the finalizing of the buffer queue.

> This does not seem ideal, and I have a fix for that, but I'm going to iterate on it a bit to see if I can simplify the logic and make sure I account for edge cases so that nothing is logged after the EOB is written and the buffer is closed.

SGTM



================
Comment at: lib/xray/xray_fdr_logging_impl.h:110-112
+// Hardcoded setting to skip logging simple functions that run less than 5 us.
+// Will probably be customized at a later time.
+const uint64_t FuncDurationThresholdMicros = 5;
----------------
We should start by making this a flag right away, so we can test effectively.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:509
+    if (NumConsecutiveFnEnters > 0 &&
+        (TSC - LastFunctionEntryTSC) < FuncDurationThresholdMicros) {
+      RecordPtr -= FunctionRecSize;
----------------
I think this is what you meant that you need to do some math with. When do you so, consider not having to use floating point math -- if we can get away with approximations and division/masking with power of twos instead, that would really help with the efficiency. Something to keep in mind but probably not worth worrying about at this point.


================
Comment at: lib/xray/xray_fdr_logging_impl.h:511-513
+      AlignedFuncStorage AlignedFuncRecordBuffer;
+      const auto &FuncRecord = *reinterpret_cast<FunctionRecord *>(
+          std::memcpy(&AlignedFuncRecordBuffer, RecordPtr, FunctionRecSize));
----------------
Why are we copying from the log buffer into the local aligned record buffer? This seems unused now.


https://reviews.llvm.org/D31345