Re-use ExecutorService to avoid creating extra threads and resource leak

[mfussenegger]

In both the signature help and in the completion resolve path ad-hoc
cached ThreadPool instances were created and not shutdown properly.

Using the same ExecutorService instance can avoid spawning new threads,
which is cheaper than re-creating new threads all time. A quick
benchmark:

	ExecutorsBenchmark.createAdhocThreadPool          avgt   10  124.947 ± 1.527  us/op
	ExecutorsBenchmark.reuseThreadPool                avgt   10   14.979 ± 1.234  us/op

But more importantly the instances weren't shutdown after use, leaking
resources.

[testforstephen]

I like the idea of reusing the thread pool. One thing with this PR is to keep it backward compatible. Currently the new constructor will break VS Code IntelliCode extension, which is calling the old constructor to instantiate a CompletionResolveHandler. (cc: @Eskibear )

Another idea is to move ExecutorService instance to BaseJDTLanguageServer, and other places can access it via getter like JavaLanguageServerPlugin.getProtocol().getCachedThreadPool(). In this way, both syntax server and standard server share the same logic, and no API changes are required in Handlers.

[mfussenegger]

Sure I can change it. I wasn't aware that API compatiblity is a concern.
That it would also increase coupling between the CompletionResolveHandler and the BaseJDTLanguageServer is no problem?

[Eskibear]

How about keeping both two implementation, so downstream extensions like IntelliCode won't break immediately. Later we can update them to call the new constructor if necessary

public CompletionResolveHandler(ExecutorService executorService, PreferenceManager manager);

public CompletionResolveHandler(PreferenceManager manager);

[testforstephen]

How about keeping both two implementation, so downstream extensions like IntelliCode won't break immediately. Later we can update them to call the new constructor if necessary

public CompletionResolveHandler(ExecutorService executorService, PreferenceManager manager);

public CompletionResolveHandler(PreferenceManager manager);

This may be an option, but it would require the downstream extensions to make the same changes again. If we could make this benefit available to downstream extensions for free, I would vote for it.

[Eskibear]

If we could make this benefit available to downstream extensions for free

Downstream extension E.g. IntelliCode is using CompletionResolveHandler(PreferenceManager manager). Can we just change the implementation of it to use a shared ExecutorService?

[testforstephen]

yes, that's what I wanted in my first comment.

[mfussenegger]

I had a quick go at this but unit tests fail because getProtocol returns null.

Are there already tests in place that set that up so I could have a look at how it's done?

[testforstephen]

You could take a loo at
https://github.com/eclipse/eclipse.jdt.ls/blob/346d6fa63d00f3a30091ac1984e1cd835b13e83f/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandlerTest.java#L521-L530

[jdneo]

@mfussenegger Will you keep working on this PR? The idea is really good and I hope we can include it.

If you do not have time to work on it recently, would you mind me to append commits to this PR?

[mfussenegger]

If you do not have time to work on it recently, would you mind me to append commits to this PR?

It might be a while until I'd get back to it, so feel free to pick it up and continue

[jdneo]

Did some test on Windows with this change.

I kept triggering the completion resolving request by iterating the completion list for 30 times, with following code:

String s;
s.|

Time cost comparation

Avg. time cost for each completion resolving request:

Before	After
53.3ms	49.8ms

The time difference is not significant, with this change, the time cost for the resolving completion is slightly slower than before.

Memory comparation

I also captured the JVM memory status during the experiment.

Before:

After:

Both case the heap size continuously increases, no significant difference can be observed about the memory cost.

But the thread number is different. Without the change, the thread number keeps growing (since each request will create a new thread pool). While with this change, we can observe that the thread number is very stable, since all the request reuses the existing thread pool.

[jdneo]

@Eskibear @testforstephen I've updated the PR, please review when you have time :)

[testforstephen]

LGTM