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

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/FS.h:91
+IntrusiveRefCntPtr<llvm::vfs::FileSystem>
+getTimeTrackingFS(std::shared_ptr<WallTimer> Timer,
+                  IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS);
----------------
kadircet wrote:
> 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.
> > 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.

Done.


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



================
Comment at: clang-tools-extra/clangd/Preamble.h:95
               const ParseInputs &Inputs, bool StoreInMemory,
+              PreambleBuildStats *Stats,
               PreambleParsedCallback PreambleCallback);
----------------
kadircet wrote:
> 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?
OK, I made it default to nullptr.

The logic behind it not being last was that it's usual (though not a hard rule) for callbacks to be the last argument, probably to make passing lambdas look nicer. Not really important.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1008
+  PreambleBuildStats Stats;
+  const bool IsFirstPreamble = !LatestBuild;
   LatestBuild = clang::clangd::buildPreamble(
----------------
kadircet wrote:
> 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.
OK, I'll drop const for now then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121712