[PATCH] D121712: [clangd] Track time spent in filesystem ops during preamble builds

Mon Mar 21 01:28:35 PDT 2022

kadircet added inline comments.

================
Comment at: clang-tools-extra/clangd/FS.h:71

+/// Used for tracking time spent in FS operations. Like llvm::Timer, but only
+/// tracks wall time, which is much cheaper.
----------------
adamcz wrote:
> kadircet wrote:
> > kadircet wrote:
> > > I don't think mentioning `FS` here is worth it. Can we rather emphasize on this being an accumulating timer?
> > what about making all of this an implementation detail of `Preamble.cpp` ? we can lift it up once we have other places that we want to make use of.
> Before I make the change, let me clarify: are you suggesting moving the whole TimedFS into Preamble.cpp?
> 
> Definitely possible.
> Before I make the change, let me clarify: are you suggesting moving the whole TimedFS into Preamble.cpp?

Yes, exactly. as neither the FS nor the walltimer is needed by anything else (and we probably want to be more cautious when we want to make use of them in other components).

================
Comment at: clang-tools-extra/clangd/FS.h:73
+/// tracks wall time, which is much cheaper.
+/// Since this is a generic timer, We may want to move this to support/ if we
+/// find a use case outside of FS time tracking.
----------------
adamcz wrote:
> kadircet wrote:
> > maybe put a `FIXME:`
> IMHO FIXME should be actionable. This is not - we shouldn't do it unless something changes. It's just a note that if someone ever wonders "could I just move this to support/ and re-use it", the answer is yes.
> 
> Does that make sense?
SG.

================
Comment at: clang-tools-extra/clangd/FS.h:82
+  void stopTime();
+  // Return total time, in seconds.
+  double getTime();
----------------
adamcz wrote:
> kadircet wrote:
> > what about returning an `int` with `ms` granularity ?
> We could, but why? llvm::Timer returns WallTime as double so this is somewhat consistent with it. Not a strong argument, of course, since that consistency doesn't really matter, but I'm not sure what benefit using int here offers?
it's mostly personal preference i suppose, it just feels reasoning about discrete durations are easier than doubles. feel free to leave as is.

================
Comment at: clang-tools-extra/clangd/FS.h:86
+private:
+  std::chrono::steady_clock::duration TotalTime;
+  std::chrono::steady_clock::time_point StartTime;
----------------
adamcz wrote:
> kadircet wrote:
> > what about just storing double/int ?
> Again, why?
> 
> (I'm not strictly against it, just trying to understand why you're asking for this)
it just feels easier to reason about builtin types, than a template alias like `std::chrono::steady_clock::duration`, at least on the interface level. but again, probably just a personal preference that I don't feel strongly about, so feel free to ignore. (usually with `what about`s i try to signal this :D)

================
Comment at: clang-tools-extra/clangd/FS.h:91
+IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+getTimeTrackingFS(std::shared_ptr<WallTimer> Timer,
+                  IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
----------------
adamcz wrote:
> kadircet wrote:
> > kadircet wrote:
> > > i feel like this deserves the `FS` specific comment mentioned above. maybe something like:
> > > ```
> > > This will record all time spent on IO operations in \p Timer.
> > > ```
> > i don't think we ever want concurrent access to Timer, i.e. startTime should never be called without a matching call to stopTime first. passing it in as a shared_ptr somehow gives the feeling that it might be shared across multiple objects, which might do whatever they want with the object.
> > maybe just pass in a reference ?
> It's not about concurrency, it's about live time. This timer needs to have the same lifetime as the entire VFS, which is also ref-counted.
> 
> Alternative would be for the TimerFS to own this timer and expose it's own getTime() method. That means TimerFS must now be visible outside of FS.cpp, but if we're moving it to Preamble.cpp per your other comment that's fine.
> It's not about concurrency, it's about live time. This timer needs to have the same lifetime as the entire VFS, which is also ref-counted.

Right, I've noticed that as I was going through the rest of the review, but forgot to delete these as it was split into two days. sorry for the churn.

> Alternative would be for the TimerFS to own this timer and expose it's own getTime() method. That means TimerFS must now be visible outside of FS.cpp, but if we're moving it to Preamble.cpp per your other comment that's fine.

yes, i think that would be a nice simplification.

================
Comment at: clang-tools-extra/clangd/Preamble.cpp:389
+      Stats != nullptr ? getTimeTrackingFS(FSTimer, StatCacheFS) : StatCacheFS;
+  FSTimer->stopTimer();
+
----------------
is this call intentional ?

================
Comment at: clang-tools-extra/clangd/Preamble.cpp:385
   auto BuiltPreamble = PrecompiledPreamble::Build(
-      CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine,
-      StatCache->getProducingFS(VFS),
+      CI, ContentsBuffer.get(), Bounds, *PreambleDiagsEngine, TimedFS,
       std::make_shared<PCHContainerOperations>(), StoreInMemory, CapturedInfo);
----------------
adamcz wrote:
> kadircet wrote:
> > it might be worth leaving some comments around this `TimedFS` being exposed to outside through PreambleCallbacks. We provide access to Preprocessor, which keeps the underlying FileManager alive.
> > Today all of this happens in serial hence all is fine, but we were discussing the possibility of performing the indexing and preamble serialization in parallel, it might result in some surprising race conditions if we do that.
> Hmm...I added a comment, but nothing really changes here, right? VFS in general is not thread safe, so if we add parallelism we need to either make sure no file access happens there or make all VFSs used here thread safe.
right. in theory we have that more loudly spelled out by having a `ThreadSafeFS`, but as you pointed out that wouldn't be the only thing to consider when such a shift happens.

================
Comment at: clang-tools-extra/clangd/Preamble.h:95
               const ParseInputs &Inputs, bool StoreInMemory,
+              PreambleBuildStats *Stats,
               PreambleParsedCallback PreambleCallback);
----------------
adamcz wrote:
> kadircet wrote:
> > nit: maybe make this the last parameter and default to `nullptr` to get rid of changes in tests.
> I'd rather not, unless you insist. Besides not having to modify tests (which I already did anyway), what's the benefit of having it be default? Do you think it's more readable?
right, i think it's more readable, and moreover it will reduce the need for typing that parameter more times in the future (mostly in tests again).
at the very least, what's the reason for not inserting it at the last position but rather before PreambleCallback?

================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:113
+
+static void reportPreambleBuild(const PreambleBuildStats &Stats,
+                                bool IsFirstPreamble) {
----------------
we can drop the `static` now.

================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:115
+                                bool IsFirstPreamble) {
+  llvm::once_flag OnceFlag;
+  llvm::call_once(OnceFlag, [&] {
----------------
doesn't this need to be at least static ?

================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:100

+// Tracks latency (in seconds) of FS operations done during a preamble build.
+// build_type allows to split by expected VFS cache state (cold on first
----------------
adamcz wrote:
> kadircet wrote:
> > can we move all of these into an anonymous namespace instead?
> Done. I moved them into anonymous namespace, but I'm not sure what you mean by "instead"?
instead of having `static` in front of the `void reportPreambleBuild` :D (sorry the comment was definitely misplaced).

================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:109
+// preamble build time. build_type is same as above.
+constexpr trace::Metric PreambleBuildFilesystemLatencyRatio(
+    "preamble_fs_latency_ratio", trace::Metric::Distribution, "build_type");
----------------
adamcz wrote:
> kadircet wrote:
> > what about just tracking the total build time here? we can get the ratio afterwards.
> How would you get the ratio then?
we could just divide the distributions point by point. it isn't as accurate since we won't have the exact association, but something we've been doing in the past.
but no matter what, i forgot that we actually track the total build time through the span in `buildPreamble` so nvm. no need to give up the accuracy here.

================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1008
+  PreambleBuildStats Stats;
+  const bool IsFirstPreamble = !LatestBuild;
   LatestBuild = clang::clangd::buildPreamble(
----------------
adamcz wrote:
> kadircet wrote:
> > nit: drop the const per style.
> Hmm...is that actually documented somewhere? There's definitely many cases of "const bool" in LLVM codebase. I think the "const" improves readability.
> is that actually documented somewhere

nothing that I can find either.

> There's definitely many cases of "const bool" in LLVM codebase. I think the "const" improves readability.

yes, but I think the majority is still not having "const" in front of bool. it's at least the convention in clangd.
I also agree that the `const` improves readability, but for a local variable it carries much less importance and being consistent with the majority of the cases here is a lot more important.
because seeing occurrences of both locals with const and non-const will eventually make the reasoning hard and each case surprising.

if you think there's enough value to having consts for locals for readability, i think we should figure out a way to make the codebase (at least the clangd part) consistent with consts first.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121712