[PATCH] D93726: [clangd] Use atomics instead of locks to track periodic memory trimming

[Sam McCall via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG3dbe471a2603: [clangd] Use atomics instead of locks to track 
periodic memory trimming (authored by sammccall).

Changed prior to commit:
  https://reviews.llvm.org/D93726?vs=313413=313422#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93726/new/

https://reviews.llvm.org/D93726

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/support/Threading.cpp
  clang-tools-extra/clangd/support/Threading.h
  clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp

Index: clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
+++ clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
@@ -10,6 +10,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 
 namespace clang {
@@ -121,5 +122,25 @@
   ASSERT_THAT(ValueA.load(), testing::AnyOf('A', 'B'));
 }
 
+// It's hard to write a real test of this class, std::chrono is awkward to mock.
+// But test some degenerate cases at least.
+TEST(PeriodicThrottlerTest, Minimal) {
+  PeriodicThrottler Once(std::chrono::hours(24));
+  EXPECT_TRUE(Once());
+  EXPECT_FALSE(Once());
+  EXPECT_FALSE(Once());
+
+  PeriodicThrottler Later(std::chrono::hours(24),
+  /*Delay=*/std::chrono::hours(24));
+  EXPECT_FALSE(Later());
+  EXPECT_FALSE(Later());
+  EXPECT_FALSE(Later());
+
+  PeriodicThrottler Always(std::chrono::seconds(0));
+  EXPECT_TRUE(Always());
+  EXPECT_TRUE(Always());
+  EXPECT_TRUE(Always());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/support/Threading.h
===
--- clang-tools-extra/clangd/support/Threading.h
+++ clang-tools-extra/clangd/support/Threading.h
@@ -169,6 +169,35 @@
   }
 };
 
+/// Used to guard an operation that should run at most every N seconds.
+///
+/// Usage:
+///   mutable PeriodicThrottler ShouldLog(std::chrono::seconds(1));
+///   void calledFrequently() {
+/// if (ShouldLog())
+///   log("this is not spammy");
+///   }
+///
+/// This class is threadsafe. If multiple threads are involved, then the guarded
+/// operation still needs to be threadsafe!
+class PeriodicThrottler {
+  using Stopwatch = std::chrono::steady_clock;
+  using Rep = Stopwatch::duration::rep;
+
+  Rep Period;
+  std::atomic Next;
+
+public:
+  /// If Period is zero, the throttler will return true every time.
+  PeriodicThrottler(Stopwatch::duration Period, Stopwatch::duration Delay = {})
+  : Period(Period.count()),
+Next((Stopwatch::now() + Delay).time_since_epoch().count()) {}
+
+  /// Returns whether the operation should run at this time.
+  /// operator() is safe to call concurrently.
+  bool operator()();
+};
+
 } // namespace clangd
 } // namespace clang
 #endif
Index: clang-tools-extra/clangd/support/Threading.cpp
===
--- clang-tools-extra/clangd/support/Threading.cpp
+++ clang-tools-extra/clangd/support/Threading.cpp
@@ -116,5 +116,17 @@
   CV.wait_until(Lock, D.time());
 }
 
+bool PeriodicThrottler::operator()() {
+  Rep Now = Stopwatch::now().time_since_epoch().count();
+  Rep OldNext = Next.load(std::memory_order_acquire);
+  if (Now < OldNext)
+return false;
+  // We're ready to run (but may be racing other threads).
+  // Work out the updated target time, and run if we successfully bump it.
+  Rep NewNext = Now + Period;
+  return Next.compare_exchange_strong(OldNext, NewNext,
+  std::memory_order_acq_rel);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -19,6 +19,7 @@
 #include "support/Context.h"
 #include "support/MemoryTree.h"
 #include "support/Path.h"
+#include "support/Threading.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
@@ -186,18 +187,12 @@
   /// Runs profiling and exports memory usage metrics if tracing is enabled and
   /// profiling hasn't happened recently.
   void maybeExportMemoryProfile();
+  PeriodicThrottler ShouldProfile;
 
   /// Run the MemoryCleanup callback if it's time.
   /// This method is thread safe.
   void maybeCleanupMemory();
-
-  /// Timepoint until which profiling is off. It is used to throttle profiling
-  /// requests.
-  std::chrono::steady_clock::time_point NextProfileTime;
-
-  /// Next time we want to call the MemoryCleanup 

[PATCH] D93726: [clangd] Use atomics instead of locks to track periodic memory trimming

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D93726#2468855 , @qchateau wrote:

> LGTM
>
> Small nits:
>
> - `operator()` is not `const` but `Next` is `mutable`, seems to me you 
> intended to have `operator()` as `const`

Oops, I originally did plan to make `operator()` const, but then decided it'd 
be clearer for the caller to make PeriodicThrottler `mutable` instead. (Because 
pretending it doesn't have state is confusing).
Removed mutable from `Next`.

> - memory orders for the atomic operations can probably be downgraded to 
> `std::memory_order_acquire`/`std::memory_order_acq_rel`. I think the first 
> load can even be `relaxed` but that I'm always careful with these

Done. I have to confess I often pretend memory orders other than seq_cst don't 
exist just to keep the cognitive load down, but this case seems clear/isolated 
enough.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93726/new/

https://reviews.llvm.org/D93726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93726: [clangd] Use atomics instead of locks to track periodic memory trimming

[Quentin Chateau via Phabricator via cfe-commits]

qchateau accepted this revision.
qchateau added a comment.
This revision is now accepted and ready to land.

LGTM

Small nits:

- `operator()` is not `const` but `Next` is `mutable`, seems to me you intended 
to have `operator()` as `const`
- memory orders for the atomic operations can probably be downgraded to 
`std::memory_order_acquire`/`std::memory_order_acq_rel`. I think the first load 
can even be `relaxed` but that I'm always careful with these


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93726/new/

https://reviews.llvm.org/D93726

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D93726: [clangd] Use atomics instead of locks to track periodic memory trimming

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: qchateau.
Herald added subscribers: usaxena95, kadircet, jfb, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.

Instead of always locking/unlocking a contended mutex, we now do one atomic read
in the common case, and one read + one exchange if the timer has expried.

Also use this for memory profiling which has similar/compatible requirements.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93726

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/support/Threading.cpp
  clang-tools-extra/clangd/support/Threading.h
  clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp

Index: clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
+++ clang-tools-extra/clangd/unittests/support/ThreadingTests.cpp
@@ -10,6 +10,7 @@
 #include "llvm/ADT/DenseMap.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
+#include 
 #include 
 
 namespace clang {
@@ -121,5 +122,25 @@
   ASSERT_THAT(ValueA.load(), testing::AnyOf('A', 'B'));
 }
 
+// It's hard to write a real test of this class, std::chrono is awkward to mock.
+// But test some degenerate cases at least.
+TEST(PeriodicThrottlerTest, Minimal) {
+  PeriodicThrottler Once(std::chrono::hours(24));
+  EXPECT_TRUE(Once());
+  EXPECT_FALSE(Once());
+  EXPECT_FALSE(Once());
+
+  PeriodicThrottler Later(std::chrono::hours(24),
+  /*Delay=*/std::chrono::hours(24));
+  EXPECT_FALSE(Later());
+  EXPECT_FALSE(Later());
+  EXPECT_FALSE(Later());
+
+  PeriodicThrottler Always(std::chrono::seconds(0));
+  EXPECT_TRUE(Always());
+  EXPECT_TRUE(Always());
+  EXPECT_TRUE(Always());
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/support/Threading.h
===
--- clang-tools-extra/clangd/support/Threading.h
+++ clang-tools-extra/clangd/support/Threading.h
@@ -169,6 +169,35 @@
   }
 };
 
+/// Used to guard an operation that should run at most every N seconds.
+///
+/// Usage:
+///   mutable PeriodicThrottler ShouldLog(std::chrono::seconds(1));
+///   void calledFrequently() {
+/// if (ShouldLog())
+///   log("this is not spammy");
+///   }
+///
+/// This class is threadsafe. If multiple threads are involved, then the guarded
+/// operation still needs to be threadsafe!
+class PeriodicThrottler {
+  using Stopwatch = std::chrono::steady_clock;
+  using Rep = Stopwatch::duration::rep;
+
+  Rep Period;
+  mutable std::atomic Next;
+
+public:
+  /// If Period is zero, the throttler will return true every time.
+  PeriodicThrottler(Stopwatch::duration Period, Stopwatch::duration Delay = {})
+  : Period(Period.count()),
+Next((Stopwatch::now() + Delay).time_since_epoch().count()) {}
+
+  /// Returns whether the operation should run at this time.
+  /// operator() is safe to call concurrently.
+  bool operator()();
+};
+
 } // namespace clangd
 } // namespace clang
 #endif
Index: clang-tools-extra/clangd/support/Threading.cpp
===
--- clang-tools-extra/clangd/support/Threading.cpp
+++ clang-tools-extra/clangd/support/Threading.cpp
@@ -116,5 +116,16 @@
   CV.wait_until(Lock, D.time());
 }
 
+bool PeriodicThrottler::operator()() {
+  Rep Now = Stopwatch::now().time_since_epoch().count();
+  Rep OldNext = Next.load();
+  if (Now < OldNext)
+return false;
+  // We're ready to run (but may be racing other threads).
+  // Work out the updated target time, and run if we successfully bump it.
+  Rep NewNext = Now + Period;
+  return Next.compare_exchange_strong(OldNext, NewNext);
+}
+
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -19,6 +19,7 @@
 #include "support/Context.h"
 #include "support/MemoryTree.h"
 #include "support/Path.h"
+#include "support/Threading.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringSet.h"
@@ -186,18 +187,12 @@
   /// Runs profiling and exports memory usage metrics if tracing is enabled and
   /// profiling hasn't happened recently.
   void maybeExportMemoryProfile();
+  PeriodicThrottler ShouldProfile;
 
   /// Run the MemoryCleanup callback if it's time.
   /// This method is thread safe.
   void maybeCleanupMemory();
-
-  /// Timepoint until which profiling is off. It is used to throttle profiling
-  /// requests.
-  std::chrono::steady_clock::time_point NextProfileTime;
-
-  /// Next time we want to call the