Run long-spanning tasks cancellably on background in the (Java) LSP server.#7708
Draft
lahodaj wants to merge 1348 commits intoapache:masterfrom
Draft
Run long-spanning tasks cancellably on background in the (Java) LSP server.#7708lahodaj wants to merge 1348 commits intoapache:masterfrom
lahodaj wants to merge 1348 commits intoapache:masterfrom
Conversation
Contributor
Author
|
I think we need to make progress on this. |
lahodaj
added
Java
lahodaj
changed the title
lahodaj
force-pushed
the
java-lsp-server-compute-long-spanning-tasks-cancellably-on-background
branch
from
a768cee to
71e959b
Compare
Contributor
Author
|
FWIW: I've changed the implementation to an explicit priority "queue", as the original patch with injecting tasks in the editor background infrastructure proved to be too complex, at least for now. |
lahodaj
force-pushed
the
java-lsp-server-compute-long-spanning-tasks-cancellably-on-background
branch
from
49bb7f8 to
a7f0d23
Compare
Contributor
Author
|
I limitation of this way to cancel things is that it cannot stop computations inside javac/parser. Unlike the previous round which used the "editor" tasks, which can stop even javac/parser. But, this patch:
|
eirikbakke
reviewed
Jan 19, 2025
Contributor
eirikbakke
left a comment
eirikbakke
left a comment
There was a problem hiding this comment.
Just passing by with some comments/questions... I'm not very familiar with this area in the codebase.
| public void cancel(); | ||
| } | ||
|
|
||
| public final class CancelCheck { |
Contributor
There was a problem hiding this comment.
Is this supposed to be a public class? It is currently used only within this class.
| } | ||
|
|
||
| public void registerCancel(CancelCallback callback) { | ||
| cancelCallback.set(callback); |
Contributor
There was a problem hiding this comment.
Perhaps throw an IllegalStateException if a callback is already registered.
| void cancel() { | ||
| cancelled.set(true); | ||
|
|
||
| CancelCallback callback = cancelCallback.get(); |
Contributor
There was a problem hiding this comment.
It would be easier to verify the thread-safety of these methods if the accesses to cancelled and cancelCallback were synchronized as a unit, rather than just being individually protected by AtomicBoolean/AtomicReference.
But in practice I assume registerCancel is going to be called up-front, before concurrent accesses start.
| private final P data; | ||
| private final CompletableFuture<R> result; | ||
|
|
||
| public TaskDescription(CancellableTask<P, R> task, P data, CompletableFuture<R> result) { |
Contributor
There was a problem hiding this comment.
Perhaps add null checks on parameters so that NPEs happen immediately and with a useful stack trace, rather than with a delay.
|
|
||
| private CancelCheck() {} | ||
|
|
||
| public boolean isCancelled() { |
Contributor
There was a problem hiding this comment.
Shouldn't this be called somewhere from TextDocumentServiceImpl? Whenever I do cancelable operations, I usually have a loop of batch operations with an if (isCanceled()) break; somewhere.
Move Splash initialization to EDT and make it no longer modal
[GITHUB-6925] Fixing debugger.jpda.truffle tests.
the tests can run on regular JDKs, GraalVM's polyglot features became dependencies in pasts. the modules are already covered by the java, debugger, ide, profiler and platform jobs (only the nashorn module needed to be moved).
The warning said "No enabled eligible for injection beans are found", because the filter class did not check "jakarta." namespace
CDI: fix injection warning for jakarta predefined bean classes
CI: Remove GraalVM job
Export-Package entries are not just comma separated values, but can contain parameters, which themselves can contain commas in quoted strings. These cases need to be handled correctly.
Move the setting of awtAppClassName for XToolkit into Main so that it is set before splash and import dialogs are created. This fixes issues with duplicate dock icons caused by incorrect WM_CLASS derived from this field value.
## Skip firing events for up-to-date Skip firing events for up-to-date files that are not yet in the cache, since UPTODATE is the default for managed files. This drastically reduces the time spent in the refreshStatusesBatch method on big repositories executed when Commit dialog opens. For example, on the Netbeans repository, from around 8 seconds to 20ms. ## Batch status change notifications Batch status change notifications to avoid per-file event overhead. Replace per-file PROP_FILE_STATUS_CHANGED firing in refreshStatusesBatch with a single PROP_FILES_STATUS_CHANGED batch event, eliminating 100k redundant propertyChange/schedule/SwingUtilities.invokeLater calls on first load. This improves performance a bit because it eliminates many method calls. However, in the end, the number of files changed is the same so the event handler still needs to process all of them. ## Move status updates to a background thread File status update requires I/O operation to refresh files metadata from FS. This is very slow when many files need to be updated. Moving them to a background thread offloads this slow operation from the main thread. Updates can run asynchronously without blocking the main thread that fires the status events, they just update UI hints, they have no impact no behavior. For the whole Netbeans repository, this shortens the time it takes to complete the firePropertyChange event from 6 seconds to 1 second.
Move setting of awtAppClassName from MainWindow to Main
Use a regular expression to not consider comma between quote while co…
make jnlp build process runnable again
- use Data*Streams instead of Object*Streams - simplify file format (no XML) - code renovation and other minor optimizations results in 4x faster cache loading times (readCache() and calculateParents() methods) minor: - ModuleManager.EnableContext: List -> Set for field solely used for contains() in inner loop - Module: data loading can synchronize on instance instead of class (DATA_LOCK no longer static) note: The IO utility class calls Dependency#create using MethodHandle to avoid having to make it public API or introduce a split package.
Improve startup cache loading performance
…the CRaCCheckpointTo Option
Improving code completion in presence of local classes.
FISH-12969 Payara Server Startup Fails in Apache NetBeans IDE Due to the CRaCCheckpointTo Option
…optimization-skip-no-update Speed up loading file statuses in Git commit dialog by batching events and skipping events for up-to-date files
Speeds up GitClient.getStatus() by deferring expensive evaluation of object Ids, which often compute file content hash, to evaluate them lazily only when needed. On Netbeans repository with a lot of files, this speeds up GitClient.getStatus() execution from 4 seconds to 1 second.
…optimization StatusCommand: Optimize git status by deferring getObjectId() calls
Newer bouncy castle JARs declare an import of the java.io package. This is rejected by the manifest verification in the equinox version NetBeans uses wrapped into Netbinox. This change deactivates the check in equinox that rejects that entry. Additionally the instructions for building the patched equinox version were updated. Closes: apache#8894
…date Enable loading of bundles importing java.-packages and update Bouncy Castle to fix jgit signing
adjusted RevWalk max flags constant since it diverges from javadoc and added tripwire test in case it changes again in future.
Update jgit from 7.2.0 to 7.6.0
…anning-tasks-cancellably-on-background' into java-lsp-server-compute-long-spanning-tasks-cancellably-on-background
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
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.
@jtulach reported the code completion inside VS Code is sometimes slow for him. I was thinking of that, and experimenting, and I think at least part of the problem is that tasks that should be background tasks are not properly cancellable in the VS Code (Java) server.
I was doing experiments on:
https://github.com/openjdk/jdk/blob/master/src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Attr.java
and it seemed there were two (current) particular pain points:
Neither of these can be cancelled when the request to compute code completion. This draft is attempting to improve that, and run diagnostics and semantic coloring in a cancellable way.
It only properly cancels for Java sources at this time, for other sources runs only the CompletableFuture cancel will work. Some work on Schedulers will be needed to fully enable this for non-Java sources, but I suspect this can wait
I also have a separate PR that adds additional logging for code completion:
#7815