Added a debounce to textDocument/didChange #1650
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I arrived at 30ms empirically starting with 5ms, 0ms, and 10ms debounce intervals.
@danielmarquespt expressed that he would prefer to have the lenses indent to the same level as the next non-blank line below them, rather than always at a fixed offset. This was very easy to add, and low-risk, so I added it. It handles errors and falls back to the old indentation if there are any problems at all. I updated the test plan to include a visual inspection for the lens indentation.
De-sync errors used to go unnoticed during testing and debugging. Now we enable `cody-agent.panic-when-out-of-sync` for `./gradlew test` and `./gradlew :runIde` to generate crashes on de-sync issues between client and agent. ## Test plan - Validated that on runIde the agent panics and logs on desync issues --------- Co-authored-by: Rik Nauta <rik.nauta@sourcegraph.com>
Instead of sending full content we are now sending only modified fragments. That should: * decrease amount of traffic * improve accuracy of the diffs (previously we were calculating them on the agent side, which sometimes cannot be guaranteed to be correct) - Manually tested all inline edit and cody completion features - Ran unit tests --------- Co-authored-by: Rik Nauta <rik.nauta@sourcegraph.com>
This commit addresses three issues with document synchronization: - Panic mode now only sends properties that are guaranteed to be correct. When paired with the changes in sourcegraph/cody#4291, this eliminates false-positive panics. Previously, the submitted information and "source of truth" information could already by out-of-sync before it even left the JetBrains client due to the multithreaded nature of IDEA. - Selection change handler now computes the selection from `SelectionEvent` instead of `Editor` to eliminate the risk that the editor's selection model changes in another thread as we're constructing parameters for textDocument/didChange. - File open handlers now use an `Editor` with the same URI as the provided `VirtualFile`. Previously, we used `selectedTextEditor` that could have a different URI. ## Test plan Manually tested locally. I can still reproduce one panic with the following minimized changes https://github.com/sourcegraph/jetbrains/assets/1408093/18ceff9d-b919-4cd4-8812-88538c5e5461 <!-- All pull requests REQUIRE a test plan: https://sourcegraph.com/docs/dev/background-information/testing_principles Why does it matter? These test plans are there to demonstrate that are following industry standards which are important or critical for our customers. They might be read by customers or an auditor. There are meant be simple and easy to read. Simply explain what you did to ensure your changes are correct! Here are a non exhaustive list of test plan examples to help you: - Making changes on a given feature or component: - "Covered by existing tests" or "CI" for the shortest possible plan if there is zero ambiguity - "Added new tests" - "Manually tested" (if non trivial, share some output, logs, or screenshot) - Updating docs: - "previewed locally" - share a screenshot if you want to be thorough - Updating deps, that would typically fail immediately in CI if incorrect - "CI" - "locally tested" --> Co-authored-by: Piotr Kukielka <pkukielka@virtuslab.com>
This bumps Cody to a commit that includes this PR here sourcegraph/cody#4291 This PR reduces the amount of false positive and lower priority crashes so that the de-sync checks can be used during reproduction of currently open P0 issues. ## Test plan n/a
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have been spamming expensive
textDocument/didChangeevents to the agent, on every selection change, cursor move, and text change.The worst offender was dragging selection changes, which would send hundreds of messages per second as you dragged the mouse. But caret changes and document changes were also spammy.
Olaf noticed this and requested a debounce with Issue 1618; this PR addresses it. The debounce is set to 30ms from experimentation, but easy to tweak.
There is also a parameter
forceSkipDebouncein the debouncer to let particular code paths skip/bypass the debounce when necessary, and make the rpc call directly.Performance
You can run a fairly simple manual test to see it in action. My test was to drag-select an entire 1000-line Java file, dragging the selection from the first line all the way to the end (the IDE scrolls as you drag), which takes about 10 seconds on my build target.
Prior to adding the debounce, my test took 11 seconds and emitted about 25,000 lines into the
cody-agent-trace.jsontrace file, which grew to 10.5mb.After the debounce, selecting the whole file still took 11 seconds (no discernible change), but only emitted 6000 lines to the trace file, which grew to -- well, 6mb. Still pretty big, because we're sending the whole 1000-line file on most of those trace lines. But better.
So the net effect for users is that we reduced the RPC traffic from Cody document synchronization by 75%, and cut the trace file size by nearly half.
Is this a hack?
This, this is a hack. We should be sending only incremental document updates, but that will wait until after GA. So this is a good interim solution. We will remove the debouncer when incremental synchronization is working well.
Test plan
This is pretty invisible to users, so I had to test it using logging statements and then doing a lot of playing with the selection, which was creating by far the most events. From what I could tell, selections got a bit faster with the debounce, especially selecting the whole file by dragging from the top.
I did add a flag, the Java system property
cody.bypassTextChangeDebounce, which can be turned on to disable the debounce and send every single textDocument/didChange event to the Agent. Set the value to "true" and restart the IDE to turn off debouncing, e.g. if you would like to compare their performance.