<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Reverted in <span style="font-family: Monaco;" class="">rL314786.</span><div class=""><font face="Monaco" class=""><br class=""></font><div style=""><blockquote type="cite" class=""><div class="">On 3 Oct 2017, at 22:39, Dean Michael Berris <<a href="mailto:dberris@google.com" class="">dberris@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class="">Hi Diana -- yes, I'll revert in the meantime.<br class=""><br class=""><blockquote type="cite" class="">On 3 Oct 2017, at 20:10, Diana Picus <<a href="mailto:diana.picus@linaro.org" class="">diana.picus@linaro.org</a>> wrote:<br class=""><br class="">Hi,<br class=""><br class="">Looks like this broke a number of bots:<br class=""><a href="http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/11102" class="">http://lab.llvm.org:8011/builders/clang-cmake-armv7-a15-full/builds/11102</a><br class="">http://lab.llvm.org:8011/builders/clang-ppc64le-linux-lnt/builds/7016/<br class="">http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-debian-fast/builds/6251<br class=""><br class="">Are you looking into it?<br class=""><br class="">Thanks,<br class="">Diana<br class=""><br class="">On 3 October 2017 at 08:15, Dean Michael Berris via llvm-commits<br class=""><llvm-commits@lists.llvm.org> wrote:<br class=""><blockquote type="cite" class="">Author: dberris<br class="">Date: Mon Oct 2 23:15:34 2017<br class="">New Revision: 314766<br class=""><br class="">URL: http://llvm.org/viewvc/llvm-project?rev=314766&view=rev<br class="">Log:<br class="">[XRay][compiler-rt] Use a hand-written circular buffer in BufferQueue<br class=""><br class="">Summary:<br class="">This change removes the dependency on using a std::deque<...> for the<br class="">storage of the buffers in the buffer queue. We instead implement a<br class="">fixed-size circular buffer that's resilient to exhaustion, and preserves<br class="">the semantics of the BufferQueue.<br class=""><br class="">We're moving away from using std::deque<...> for two reasons:<br class=""><br class=""> - We want to remove dependencies on the STL for data structures.<br class=""><br class=""> - We want the data structure we use to not require re-allocation in<br class=""> the normal course of operation.<br class=""><br class="">The internal implementation of the buffer queue uses heap-allocated<br class="">arrays that are initialized once when the BufferQueue is created, and<br class="">re-uses slots in the buffer array as buffers are returned in order.<br class=""><br class="">We also change the lock used in the implementation to a spinlock<br class="">instead of a blocking mutex. We reason that since the release operations<br class="">now take very little time in the critical section, that a spinlock would<br class="">be appropriate.<br class=""><br class="">This change is related to D38073.<br class=""><br class="">Reviewers: dblaikie, kpw, pelikan<br class=""><br class="">Subscribers: llvm-commits<br class=""><br class="">Differential Revision: https://reviews.llvm.org/D38119<br class=""><br class="">Modified:<br class=""> compiler-rt/trunk/lib/xray/xray_buffer_queue.cc<br class=""> compiler-rt/trunk/lib/xray/xray_buffer_queue.h<br class=""> compiler-rt/trunk/test/xray/TestCases/Linux/fdr-thread-order.cc<br class=""><br class="">Modified: compiler-rt/trunk/lib/xray/xray_buffer_queue.cc<br class="">URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/xray/xray_buffer_queue.cc?rev=314766&r1=314765&r2=314766&view=diff<br class="">==============================================================================<br class="">--- compiler-rt/trunk/lib/xray/xray_buffer_queue.cc (original)<br class="">+++ compiler-rt/trunk/lib/xray/xray_buffer_queue.cc Mon Oct 2 23:15:34 2017<br class="">@@ -16,6 +16,7 @@<br class="">#include "sanitizer_common/sanitizer_common.h"<br class="">#include "sanitizer_common/sanitizer_libc.h"<br class=""><br class="">+#include <algorithm><br class="">#include <cstdlib><br class="">#include <tuple><br class=""><br class="">@@ -23,18 +24,21 @@ using namespace __xray;<br class="">using namespace __sanitizer;<br class=""><br class="">BufferQueue::BufferQueue(std::size_t B, std::size_t N, bool &Success)<br class="">- : BufferSize(B), Buffers(N), Mutex(), OwnedBuffers(), Finalizing{0} {<br class="">- for (auto &T : Buffers) {<br class="">+ : BufferSize(B), Buffers(new std::tuple<Buffer, bool>[N]()),<br class="">+ BufferCount(N), Finalizing{0}, OwnedBuffers(new void *[N]()),<br class="">+ Next(Buffers.get()), First(nullptr) {<br class="">+ for (size_t i = 0; i < N; ++i) {<br class="">+ auto &T = Buffers[i];<br class=""> void *Tmp = malloc(BufferSize);<br class=""> if (Tmp == nullptr) {<br class=""> Success = false;<br class=""> return;<br class=""> }<br class="">-<br class=""> auto &Buf = std::get<0>(T);<br class="">+ std::get<1>(T) = false;<br class=""> Buf.Buffer = Tmp;<br class=""> Buf.Size = B;<br class="">- OwnedBuffers.emplace(Tmp);<br class="">+ OwnedBuffers[i] = Tmp;<br class=""> }<br class=""> Success = true;<br class="">}<br class="">@@ -42,27 +46,43 @@ BufferQueue::BufferQueue(std::size_t B,<br class="">BufferQueue::ErrorCode BufferQueue::getBuffer(Buffer &Buf) {<br class=""> if (__sanitizer::atomic_load(&Finalizing, __sanitizer::memory_order_acquire))<br class=""> return ErrorCode::QueueFinalizing;<br class="">- __sanitizer::BlockingMutexLock Guard(&Mutex);<br class="">- if (Buffers.empty())<br class="">+ __sanitizer::SpinMutexLock Guard(&Mutex);<br class="">+<br class="">+ if (Next == First)<br class=""> return ErrorCode::NotEnoughMemory;<br class="">- auto &T = Buffers.front();<br class="">+<br class="">+ auto &T = *Next;<br class=""> auto &B = std::get<0>(T);<br class=""> Buf = B;<br class="">- B.Buffer = nullptr;<br class="">- B.Size = 0;<br class="">- Buffers.pop_front();<br class="">+<br class="">+ if (First == nullptr)<br class="">+ First = Next;<br class="">+ ++Next;<br class="">+ if (Next == (Buffers.get() + BufferCount))<br class="">+ Next = Buffers.get();<br class="">+<br class=""> return ErrorCode::Ok;<br class="">}<br class=""><br class="">BufferQueue::ErrorCode BufferQueue::releaseBuffer(Buffer &Buf) {<br class="">- if (OwnedBuffers.count(Buf.Buffer) == 0)<br class="">+ // Blitz through the buffers array to find the buffer.<br class="">+ if (std::none_of(OwnedBuffers.get(), OwnedBuffers.get() + BufferCount,<br class="">+ [&Buf](void *P) { return P == Buf.Buffer; }))<br class=""> return ErrorCode::UnrecognizedBuffer;<br class="">- __sanitizer::BlockingMutexLock Guard(&Mutex);<br class="">+ __sanitizer::SpinMutexLock Guard(&Mutex);<br class="">+<br class="">+ // This points to a semantic bug, we really ought to not be releasing more<br class="">+ // buffers than we actually get.<br class="">+ if (First == nullptr || First == Next)<br class="">+ return ErrorCode::NotEnoughMemory;<br class=""><br class=""> // Now that the buffer has been released, we mark it as "used".<br class="">- Buffers.emplace(Buffers.end(), Buf, true /* used */);<br class="">+ *First = std::make_tuple(Buf, true);<br class=""> Buf.Buffer = nullptr;<br class=""> Buf.Size = 0;<br class="">+ ++First;<br class="">+ if (First == (Buffers.get() + BufferCount))<br class="">+ First = Buffers.get();<br class=""> return ErrorCode::Ok;<br class="">}<br class=""><br class="">@@ -74,7 +94,8 @@ BufferQueue::ErrorCode BufferQueue::fina<br class="">}<br class=""><br class="">BufferQueue::~BufferQueue() {<br class="">- for (auto &T : Buffers) {<br class="">+ for (auto I = Buffers.get(), E = Buffers.get() + BufferCount; I != E; ++I) {<br class="">+ auto &T = *I;<br class=""> auto &Buf = std::get<0>(T);<br class=""> free(Buf.Buffer);<br class=""> }<br class=""><br class="">Modified: compiler-rt/trunk/lib/xray/xray_buffer_queue.h<br class="">URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/xray/xray_buffer_queue.h?rev=314766&r1=314765&r2=314766&view=diff<br class="">==============================================================================<br class="">--- compiler-rt/trunk/lib/xray/xray_buffer_queue.h (original)<br class="">+++ compiler-rt/trunk/lib/xray/xray_buffer_queue.h Mon Oct 2 23:15:34 2017<br class="">@@ -17,8 +17,8 @@<br class=""><br class="">#include "sanitizer_common/sanitizer_atomic.h"<br class="">#include "sanitizer_common/sanitizer_mutex.h"<br class="">-#include <deque><br class="">-#include <unordered_set><br class="">+#include <cstdint><br class="">+#include <memory><br class="">#include <utility><br class=""><br class="">namespace __xray {<br class="">@@ -36,15 +36,23 @@ public:<br class=""> };<br class=""><br class="">private:<br class="">+ // Size of each individual Buffer.<br class=""> size_t BufferSize;<br class=""><br class=""> // We use a bool to indicate whether the Buffer has been used in this<br class=""> // freelist implementation.<br class="">- std::deque<std::tuple<Buffer, bool>> Buffers;<br class="">- __sanitizer::BlockingMutex Mutex;<br class="">- std::unordered_set<void *> OwnedBuffers;<br class="">+ std::unique_ptr<std::tuple<Buffer, bool>[]> Buffers;<br class="">+ size_t BufferCount;<br class="">+<br class="">+ __sanitizer::SpinMutex Mutex;<br class=""> __sanitizer::atomic_uint8_t Finalizing;<br class=""><br class="">+ // Sorted buffer pointers, making it quick to find buffers that we own.<br class="">+ std::unique_ptr<void *[]> OwnedBuffers;<br class="">+<br class="">+ std::tuple<Buffer, bool> *Next;<br class="">+ std::tuple<Buffer, bool> *First;<br class="">+<br class="">public:<br class=""> enum class ErrorCode : unsigned {<br class=""> Ok,<br class="">@@ -117,8 +125,9 @@ public:<br class=""> /// Buffer is marked 'used' (i.e. has been the result of getBuffer(...) and a<br class=""> /// releaseBuffer(...) operation).<br class=""> template <class F> void apply(F Fn) {<br class="">- __sanitizer::BlockingMutexLock G(&Mutex);<br class="">- for (const auto &T : Buffers) {<br class="">+ __sanitizer::SpinMutexLock G(&Mutex);<br class="">+ for (auto I = Buffers.get(), E = Buffers.get() + BufferCount; I != E; ++I) {<br class="">+ const auto &T = *I;<br class=""> if (std::get<1>(T))<br class=""> Fn(std::get<0>(T));<br class=""> }<br class=""><br class="">Modified: compiler-rt/trunk/test/xray/TestCases/Linux/fdr-thread-order.cc<br class="">URL: http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/test/xray/TestCases/Linux/fdr-thread-order.cc?rev=314766&r1=314765&r2=314766&view=diff<br class="">==============================================================================<br class="">--- compiler-rt/trunk/test/xray/TestCases/Linux/fdr-thread-order.cc (original)<br class="">+++ compiler-rt/trunk/test/xray/TestCases/Linux/fdr-thread-order.cc Mon Oct 2 23:15:34 2017<br class="">@@ -1,37 +1,63 @@<br class="">// RUN: %clangxx_xray -g -std=c++11 %s -o %t<br class="">// RUN: rm fdr-thread-order.* || true<br class="">-// RUN: XRAY_OPTIONS="patch_premain=false xray_naive_log=false xray_logfile_base=fdr-thread-order. xray_fdr_log=true verbosity=1 xray_fdr_log_func_duration_threshold_us=0" %run %t 2>&1 | FileCheck %s<br class="">-// RUN: %llvm_xray convert --symbolize --output-format=yaml -instr_map=%t "`ls fdr-thread-order.* | head -1`" | FileCheck %s --check-prefix TRACE<br class="">+// RUN: XRAY_OPTIONS="patch_premain=false xray_naive_log=false \<br class="">+// RUN: xray_logfile_base=fdr-thread-order. xray_fdr_log=true verbosity=1 \<br class="">+// RUN: xray_fdr_log_func_duration_threshold_us=0" %run %t 2>&1 | \<br class="">+// RUN: FileCheck %s<br class="">+// RUN: %llvm_xray convert --symbolize --output-format=yaml -instr_map=%t \<br class="">+// RUN: "`ls fdr-thread-order.* | head -1`"<br class="">+// RUN: %llvm_xray convert --symbolize --output-format=yaml -instr_map=%t \<br class="">+// RUN: "`ls fdr-thread-order.* | head -1`" | \<br class="">+// RUN: FileCheck %s --check-prefix TRACE<br class="">// RUN: rm fdr-thread-order.*<br class="">// FIXME: Make llvm-xray work on non-x86_64 as well.<br class="">// REQUIRES: x86_64-linux<br class="">// REQUIRES: built-in-llvm-tree<br class=""><br class="">#include "xray/xray_log_interface.h"<br class="">-#include <thread><br class="">+#include <atomic><br class="">#include <cassert><br class="">+#include <thread><br class=""><br class="">constexpr auto kBufferSize = 16384;<br class="">constexpr auto kBufferMax = 10;<br class=""><br class="">-thread_local uint64_t var = 0;<br class="">-[[clang::xray_always_instrument]] void __attribute__((noinline)) f1() { ++var; }<br class="">-[[clang::xray_always_instrument]] void __attribute__((noinline)) f2() { ++var; }<br class="">+std::atomic<uint64_t> var{0};<br class="">+<br class="">+[[clang::xray_always_instrument]] void __attribute__((noinline)) f1() {<br class="">+ for (auto i = 0; i < 1 << 20; ++i)<br class="">+ ++var;<br class="">+}<br class="">+<br class="">+[[clang::xray_always_instrument]] void __attribute__((noinline)) f2() {<br class="">+ for (auto i = 0; i < 1 << 20; ++i)<br class="">+ ++var;<br class="">+}<br class=""><br class="">int main(int argc, char *argv[]) {<br class=""> using namespace __xray;<br class=""> FDRLoggingOptions Options;<br class="">+ __xray_patch();<br class=""> assert(__xray_log_init(kBufferSize, kBufferMax, &Options,<br class=""> sizeof(FDRLoggingOptions)) ==<br class=""> XRayLogInitStatus::XRAY_LOG_INITIALIZED);<br class="">- __xray_patch();<br class="">- std::thread t1([] { f1(); });<br class="">- std::thread t2([] { f2(); });<br class="">- t1.join();<br class="">- t2.join();<br class="">+<br class="">+ std::atomic_thread_fence(std::memory_order_acq_rel);<br class="">+<br class="">+ {<br class="">+ std::thread t1([] { f1(); });<br class="">+ std::thread t2([] { f2(); });<br class="">+ t1.join();<br class="">+ t2.join();<br class="">+ }<br class="">+<br class="">+ std::atomic_thread_fence(std::memory_order_acq_rel);<br class=""> __xray_log_finalize();<br class=""> __xray_log_flushLog();<br class="">+ __xray_unpatch();<br class="">+ return var > 0 ? 0 : 1;<br class=""> // CHECK: {{.*}}XRay: Log file in '{{.*}}'<br class="">+ // CHECK-NOT: Failed<br class="">}<br class=""><br class="">// We want to make sure that the order of the function log doesn't matter.<br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class="">llvm-commits@lists.llvm.org<br class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits<br class=""></blockquote></blockquote><br class=""></div></div></blockquote></div><br class=""></div></body></html>