[clang-tools-extra] [clang-doc] Improve performance by adding a short circuit (PR #96809)

[Paul Kirth via cfe-commits]

================
@@ -34,13 +43,26 @@ template <typename T> bool MapASTVisitor::mapDecl(const T *D) {
   // If there is an error generating a USR for the decl, skip this decl.
   if (index::generateUSRForDecl(D, USR))
     return true;
+
+  // Prevent Visiting USR twice
+  {
+    std::lock_guard<llvm::sys::Mutex> Guard(USRVisitedGuard);
+    std::string Visited = USR.str().str();
+    if (USRVisited.count(Visited))
+      return true;
+    // We considered a USR to be visited only when its defined
+    if (IsDefinition)
+      USRVisited.insert(Visited);
----------------
ilovepi wrote:

I think this is why we need more tests that exercise a diverse set of behavior. Some of the different targets should control the documentation bits w/ defines, and we should also make sure that we're covering all the ways a USR could be brought in.  For instance, if there was only one base class that brought in `Shape`, I'm not sure how much better you could do, since I'd assume it would only be included once, and then parsed in full the one time.

For more complicated arrangements, we'd need to be sure the definitions, and the documentation would be resolved the same way in all cases. I'm not aware of a mechanism that does that off the top of my head, but I would expect the `clangd` indexing mechanic probably has a way to handle this on some level. I'd also expect that the logic in `scan-deps` must have some mechanism for not processing files multiple times.

I'm not confident enough to say what you're proposing is outright wrong, but I'm also not confident that its correct, either. What I'm saying is that we need to be sure, that when we see a definition in `clang-doc` that it won't change from out beneath us. That said, given the merge logic in the reduction phase, perhaps if the USR is complete(i.e. has no missing fields), the memoization is sufficient.

https://github.com/llvm/llvm-project/pull/96809