[circt-lsp-verilog] "Debounce" onDidChange calls; update in worker

[Scheremo]

This commit introduces onChange debouncing and thread-safe diagnostics handling
for the Verilog LSP server. The main motivation for this is project-scale LSP work,
where many files are needed per compilation, and many file buffers are open at the same
time; this PR limits the recompile frequency from per-keystroke to per time bin.
It also runs the re-elaboration in a separate thread, which avoids locking down everything during elaboration.

• Introduces LSPServerOptions (with disableDebounce, debounceMinMs, debounceMaxMs) and threads it through:

  • CirctVerilogLspServerMain signature now takes (const LSPServerOptions &, const VerilogServerOptions &, JSONTransport &).
  • runVerilogLSPServer signature updated similarly.

• Adds debounced change handling via new utility:

  • New Utils/PendingChanges.h with:
    • DebounceOptions (+ factory fromLSPOptions).
    • PendingChanges (accumulated edits + timestamps).
    • PendingChangesMap (thread-safe accumulator + pool-based debounceAndThen / debounceAndUpdate).
  • LSPServer now owns PendingChangesMap + DebounceOptions.

• Makes diagnostic publishing thread-safe:

  • LSPServer adds sendDiagnostics() (mutex-guarded) and a diagnosticsMutex.

• Changes document change flow:

  • onDocumentDidChange now calls pendingChanges.debounceAndUpdate(...) and only rebuilds/publishes when the debounce callback fires (skips obsolete updates).

• Adds CLI flags to tune debouncing in circt-verilog-lsp-server.cpp:

  • --no-debounce, --debounce-min-ms, --debounce-max-ms.
  • Test mode (--test) forces synchronous behavior (no debounce).

• Improves VerilogTextFile thread-safety and lifetime:

  • Protects contents and document with std::shared_mutex.
  • Switches document to std::shared_ptr, adds getDocument() / setDocument().
  • update() now locks, applies changes, and reinitializes via initialize().
  • Call sites use getDocument() before accessing.

• Wires LSP options into server construction:

  • CirctVerilogLspServerMain.cpp passes LSPServerOptions into runVerilogLSPServer.
  • LSPServer constructor accepts LSPServerOptions, builds DebounceOptions from it.

• Adds unit tests for debouncing:

  • New unittests/Tools/circt-verilog-lsp-server/Utils/PendingChangesTest.cpp.
  • Tests immediate flush (no debounce), quiet-window flush, obsolescence when newer edits arrive, cap-based flush during continuous typing, and missing-key behavior.
  • CMake wiring for tests (unittests/*/CMakeLists.txt) and a header-only utils target (CIRCTVerilogLspUtilsHeaders).

• Minor/CMake updates:

  • Exposes Utils headers to tests via interface target.
  • Includes Utils/PendingChanges.h in the Utils library.
  • Small includes and namespace adjustments in LSPServer.cpp/.h.

• Smooths out rapid change events into a single rebuild pass.

• Prevents transport corruption by serializing outbound notifications.

• Eliminates race conditions on document lifetime during concurrent access.

Currently the VerilogTextfile resolves definition/reference calls on the
"current" VerilogDocument without taking into account changes that have been committed
in the meantime. It might lead to a nicer user experience to heuristically
update the in-flight definition/reference requests with the changebuffer queue.

[uenoku]

Super cool, I didn't think about debounce but definitely right direction for scalability! I'll try taking a look at more closely.

FYI clangd has similar mechanism so maybe we can learn the architecture https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/TUScheduler.h

[Scheremo]

Super cool, I didn't think about debounce but definitely right direction for scalability! I'll try taking a look at more closely.

FYI clangd has similar mechanism so maybe we can learn the architecture https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/TUScheduler.h

Nice! I think we are already somewhat close; clangd uses similar debouncing semantics (minimum delay, maximum wait) with adaptive resizing of those delays. It also uses a centralized thread pool to control concurrency. I think we can either push all of this into this PR, or separate it out into individual commits. I think separating it might be nicer, since this PR is already quite large and cerebral 😅 WDYT?

[uenoku]

Thank you for taking a look! Yes let's separate commits.

[Scheremo]

Thank you for taking a look! Yes let's separate commits.

@uenoku, If you have some more time, please have another look 😄 I factored out the actual scheduling so we can later plug this into a threadpool and refine debouncing semantics further; the LSPServer-side interface should only change marginally. Also added some unit tests. I'd be very grateful about any and all pointers about GTest integration; I gave it a go, but maybe there is a better way.

[uenoku]

Cool! Will take a look!

https://github.com/llvm/circt/tree/main/unittests has gtests unittests so writing unittests under that directories would be easier (maybe unittests/Tools/circt-verilog-lsp-server). One tricky thing is it's probably necessary to move header files under top-level include so they are visible from the existing unittests directory. I think we can put these headers under either circt/Support or circt/Tools/circt-verilog-lsp-server/ (if the files are generic enough we can put to circt/Support but circt/Tools/circt-verilog-lsp-server/ might be better if they are specific to LSP).

[Scheremo]

Cool! Will take a look!

https://github.com/llvm/circt/tree/main/unittests has gtests unittests so writing unittests under that directories would be easier (maybe unittests/Tools/circt-verilog-lsp-server). One tricky thing is it's probably necessary to move header files under top-level include so they are visible from the existing unittests directory. I think we can put these headers under either circt/Support or circt/Tools/circt-verilog-lsp-server/ (if the files are generic enough we can put to circt/Support but circt/Tools/circt-verilog-lsp-server/ might be better if they are specific to LSP).

I moved the tests to unittests/Tools/circt-verilog-lsp-server/Utils and (until we figure out a better solution) added an INTERFACE library for unit testing that points to the Utils directory. The setup seems to work okay.

In my opinion I would leave the header files in Utils unless any other use-case comes to mind; I think we can always "promote" them to something more broadly visible if we find applications. WDYT?

[uenoku]

Are we using cancelUpdate and deb->cancel in non-test code?

[Scheremo]

Yes, it's used onShutdown to cancel running updates and through erase of BucketRegistry in onDocumentDidClose for the same reason 😄

[Scheremo]

added an INTERFACE library for unit testing that points to the Utils

I think this makes sense.

Thank you for excellent work and I think this looks awesome! I'm still trying to understand the logic but I'm wondering if it's possible to simplify the code structure and concurrency. I'm a bit concerned about the complexity and maintainability ;) I have a few questions:

• Is template necessary for ChangeBuffer and BucketRegistry? I'm not sure if EventT could be other than TextDocumentContentChangeEvent in LSP context. It helps to reduce cognitive load :)
• Any chance a thread pool simplifies the concurrency and resource management quite a bit(like alive and cancelAll etc)? Basically something like:

threadPool.async([this, uri, timestamp] {
  std::this_thread::sleep_for(std::chrono::milliseconds(debounceMs));

  // Check if this change is still the latest
  if (isStillLatest(uri, timestamp)) {
    processDocument(uri);
  }
});

I think this would eliminate concerns related to detached thread managements and shared_ptr/weak_ptr lifetime because detached threads don't outlive the objects they reference. With a thread pool, the lifetime is managed since the thread pool can be properly shut down, waiting for all tasks to complete.

I think the debouncing can be achieved simply with timestamps as well: each change gets a timestamp, workers sleep for the debounce delay, then check if the timestamp is latest. This is similar to clangd's TUScheduler.

I think the core code would be something like:

struct LSPServer {
struct PendingChanges { // probably equal to DocChangeBucket 
  std::vector<TextDocumentContentChangeEvent> changes;
  int64_t version = 0;
  std::chrono::steady_clock::time_point firstChangeTime;
  std::chrono::steady_clock::time_point lastChangeTime;
};

StringMap<PendingChanges> pending;
std::mutex pendingMutex;

void LSPServer::onDocumentDidChange(const DidChangeTextDocumentParams &params) {
  auto now = std::chrono::steady_clock::now();
  // Record this change with a timestamp
  {
    std::lock_guard lock(pendingMutex);
    auto &pending = pendingChanges[params.textDocument.uri.file().str()];
    pending.changes.insert(pending.changes.end(),
                          params.contentChanges.begin(),
                          params.contentChanges.end());
    pending.version = params.textDocument.version;
    pending.lastChangeTime = now;  // Track when this change happened
    if (pending.changes.size() == params.contentChanges.size()) {
      pending.firstChangeTime = now;  // For max wait feature
    }
  }

  // Schedule debounced processing
  threadPool.async([this, uri=params.textDocument.uri, scheduleTime=now, options] {
    // Wait for the debounce delay
    if (!options.disableDebounce)
        std::this_thread::sleep_for(std::chrono::milliseconds(options.debounceMinMs));

    // Check if this is still the latest change
    std::vector<TextDocumentContentChangeEvent> allChanges;
    int64_t version;
    bool shouldProcess = false;

    {
      std::lock_guard lock(pendingMutex);
      auto it = pendingChanges.find(uri.file().str());
      if (it == pendingChanges.end()) return;  // Document closed

      auto &pending = it->second;
      auto now = std::chrono::steady_clock::now();

      // Check if this is still the latest change
      bool isLatest = (pending.lastChangeTime == scheduleTime);

      // Check max wait timeout
      bool maxWaitExpired = false;
      if (options.debounceMaxMs > 0) {
        auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
            now - pending.firstChangeTime);
        maxWaitExpired = elapsed.count() >= options.debounceMaxMs;
      }

      shouldProcess = options.disableDebounce || isLatest || maxWaitExpired;

      if (shouldProcess) {
        allChanges = std::move(pending.changes);
        version = pending.version;
        pending.changes.clear();
        // Reset for next batch
        if (maxWaitExpired) {
          pending.firstChangeTime = now;
        }
      }
    }

    if (!shouldProcess) return;  // Newer change came in, skip this one

    PublishDiagnosticsParams diagParams(uri, version);
    server.updateDocument(uri, allChanges, version, diagParams.diagnostics);

    std::lock_guard lock(diagnosticsMutex);
    publishDiagnostics(diagParams);
  }
}

I haven't checked this code on large designs with frequent modification so it's possible that more fine-grained parallelism might be necessary for scalability, but I'm wondering something simpler still works. This is just a suggestion and I'd like to hear your thoughts!

Thanks for giving this a closer look! I appreciate the feedback. I think your concern is valid and it's possible we don't need fine-grained concurrency / multiple docs updating at the same time, I didn't really profile this 😅. What you sketched seems reasonable to me; the main concern I have is that multiple (more or less useless) re-compilations might happen in parallel (e.g. if min delay / max wait are set too optimistically). What we can do is go full-on clangd TUSchedule and also implement the scaling of min delay and max wait according to a fixed ratio between update frequency and (measured) compile time, so such problems stay transient. We can also live with it for the moment and tackle it in a follow-up. I don't have major intuitive performance concerns with the solution you sketched.

One way or another I would suggest to extract the asynchronous scheduling parts of this so we can unit test them on their own - I'll give it a go, and we can see which version we like best 😄

[Scheremo]

added an INTERFACE library for unit testing that points to the Utils

I think this makes sense.
Thank you for excellent work and I think this looks awesome! I'm still trying to understand the logic but I'm wondering if it's possible to simplify the code structure and concurrency. I'm a bit concerned about the complexity and maintainability ;) I have a few questions:

• Is template necessary for ChangeBuffer and BucketRegistry? I'm not sure if EventT could be other than TextDocumentContentChangeEvent in LSP context. It helps to reduce cognitive load :)
• Any chance a thread pool simplifies the concurrency and resource management quite a bit(like alive and cancelAll etc)? Basically something like:

threadPool.async([this, uri, timestamp] {
  std::this_thread::sleep_for(std::chrono::milliseconds(debounceMs));

  // Check if this change is still the latest
  if (isStillLatest(uri, timestamp)) {
    processDocument(uri);
  }
});

I think this would eliminate concerns related to detached thread managements and shared_ptr/weak_ptr lifetime because detached threads don't outlive the objects they reference. With a thread pool, the lifetime is managed since the thread pool can be properly shut down, waiting for all tasks to complete.
I think the debouncing can be achieved simply with timestamps as well: each change gets a timestamp, workers sleep for the debounce delay, then check if the timestamp is latest. This is similar to clangd's TUScheduler.
I think the core code would be something like:

struct LSPServer {
struct PendingChanges { // probably equal to DocChangeBucket 
  std::vector<TextDocumentContentChangeEvent> changes;
  int64_t version = 0;
  std::chrono::steady_clock::time_point firstChangeTime;
  std::chrono::steady_clock::time_point lastChangeTime;
};

StringMap<PendingChanges> pending;
std::mutex pendingMutex;

void LSPServer::onDocumentDidChange(const DidChangeTextDocumentParams &params) {
  auto now = std::chrono::steady_clock::now();
  // Record this change with a timestamp
  {
    std::lock_guard lock(pendingMutex);
    auto &pending = pendingChanges[params.textDocument.uri.file().str()];
    pending.changes.insert(pending.changes.end(),
                          params.contentChanges.begin(),
                          params.contentChanges.end());
    pending.version = params.textDocument.version;
    pending.lastChangeTime = now;  // Track when this change happened
    if (pending.changes.size() == params.contentChanges.size()) {
      pending.firstChangeTime = now;  // For max wait feature
    }
  }

  // Schedule debounced processing
  threadPool.async([this, uri=params.textDocument.uri, scheduleTime=now, options] {
    // Wait for the debounce delay
    if (!options.disableDebounce)
        std::this_thread::sleep_for(std::chrono::milliseconds(options.debounceMinMs));

    // Check if this is still the latest change
    std::vector<TextDocumentContentChangeEvent> allChanges;
    int64_t version;
    bool shouldProcess = false;

    {
      std::lock_guard lock(pendingMutex);
      auto it = pendingChanges.find(uri.file().str());
      if (it == pendingChanges.end()) return;  // Document closed

      auto &pending = it->second;
      auto now = std::chrono::steady_clock::now();

      // Check if this is still the latest change
      bool isLatest = (pending.lastChangeTime == scheduleTime);

      // Check max wait timeout
      bool maxWaitExpired = false;
      if (options.debounceMaxMs > 0) {
        auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(
            now - pending.firstChangeTime);
        maxWaitExpired = elapsed.count() >= options.debounceMaxMs;
      }

      shouldProcess = options.disableDebounce || isLatest || maxWaitExpired;

      if (shouldProcess) {
        allChanges = std::move(pending.changes);
        version = pending.version;
        pending.changes.clear();
        // Reset for next batch
        if (maxWaitExpired) {
          pending.firstChangeTime = now;
        }
      }
    }

    if (!shouldProcess) return;  // Newer change came in, skip this one

    PublishDiagnosticsParams diagParams(uri, version);
    server.updateDocument(uri, allChanges, version, diagParams.diagnostics);

    std::lock_guard lock(diagnosticsMutex);
    publishDiagnostics(diagParams);
  }
}

I haven't checked this code on large designs with frequent modification so it's possible that more fine-grained parallelism might be necessary for scalability, but I'm wondering something simpler still works. This is just a suggestion and I'd like to hear your thoughts!

Thanks for giving this a closer look! I appreciate the feedback. I think your concern is valid and it's possible we don't need fine-grained concurrency / multiple docs updating at the same time, I didn't really profile this 😅. What you sketched seems reasonable to me; the main concern I have is that multiple (more or less useless) re-compilations might happen in parallel (e.g. if min delay / max wait are set too optimistically). What we can do is go full-on clangd TUSchedule and also implement the scaling of min delay and max wait according to a fixed ratio between update frequency and (measured) compile time, so such problems stay transient. We can also live with it for the moment and tackle it in a follow-up. I don't have major intuitive performance concerns with the solution you sketched.

One way or another I would suggest to extract the asynchronous scheduling parts of this so we can unit test them on their own - I'll give it a go, and we can see which version we like best 😄

I built a version of the structure you suggested; I took the liberty to pack it into a data structure that lives out LSPServer (PendingChanges.h) and keep the semantics of "enqueuing a change" and "debounce and then run my callback".
I did that because it makes unit testing much easier; I added some tests to make sure the algorithm correctly debounces.

I also took the liberty of extracting construction of DebounceOptions, so we can extend them in a follow-up without having to change the LSPServer.

I also moved the handling of un-debounced calls back to the main thread; this is needed for the textdocument-didchange.test test to succeed, since it doesn't allow for the task switch to take any amount of time 😄

I like this version better; I think you were right, it has less surface area for things to go wrong.

[uenoku]

LGTM, thank you for updating code structure and adding comments about mutex ownership :) It makes much easier to follow the logic. Mostly minor code style comment.

[uenoku]

Cool!