julia-vscode / SymbolServer.jl

Other
23 stars 31 forks source link

Improve performance with `recursive_copy` #159

Closed timholy closed 4 years ago

timholy commented 4 years ago

I decided to see if I could determine whether there was any way to speed up LanguageServer. I'm not sure how to run it in "real" circumstances, but as a proxy I decided to profile the various files in its tests. It turns out there was exactly one obvious bottleneck: calls to deepcopy(SymbolServer.stdlibs).

As a comment in this PR explains, deepcopy is safe and convenient but slow; its slowness comes from two factors:

If you don't need to worry about cycles, you can do a lot better by defining your own recursive copy function, and that is done in this PR. There is a downside, mainly that as you modify types it's important to remember to update your implementation of recursive_copy, and new types require methods of their own. But the advantage is improved performance. For instance, with this PR and if you make the single change

diff --git a/src/languageserverinstance.jl b/src/languageserverinstance.jl
index 7cdbe4e..1852246 100644
--- a/src/languageserverinstance.jl
+++ b/src/languageserverinstance.jl
@@ -70,7 +70,7 @@ mutable struct LanguageServerInstance
             depot_path,
             SymbolServer.SymbolServerInstance(depot_path, symserver_store_path),
             Channel(Inf),
-            deepcopy(SymbolServer.stdlibs),
+            SymbolServer.recursive_copy(SymbolServer.stdlibs),
             SymbolServer.collect_extended_methods(SymbolServer.stdlibs),
             false,
             DocumentFormat.FormatOptions(),
@@ -321,5 +321,3 @@ function Base.run(server::LanguageServerInstance)
         end
     end
 end

in LanguageServer.jl, then I get these benchmarks on LanguageServer's tests (run after test_communication since the other tests seem to use its results):

Test script Time, master Time, this PR Gain
test_intellisense.jl 0.40s 0.04s 10x
test_edit 0.6s 0.2s 3x
test_actions 0.4s 0.035s 11x
test_paths 0.02s 0.02s no change

As you can see, these are nothing to sneeze at. I am not sure whether these will translate into similar gains in real-world circumstances; is there a simple way to trigger indexing on a specific package from the REPL? I'd be happy to take a poke at that and see if there are more performance gains to be had.

ZacLN commented 4 years ago

Thank this looks fantastic!

With regards to the timings and how this translates to real world responsiveness - the copying of the SymbolServer.stdlibs store is a once per session operation so this change will benefit the startup time for the language server. The intention is to fiddle with the SymbolServer storage objects' structure as little as possible so maintaining the recursive_copy functions isn't an issue.

The following snippet would time the indexing of a package:

using SymbolServer
using Packages,You,Want,To,Index
env = SymbolServer.getenvtree() #This creates a tree of all modules within the current session, including submodules
@time SymbolServer.symbols(env) # index everything
@time SymbolServer.symbols(env, somemodule) # index a single module

For realistic timings I think you need to restart the Julia session between runs, though for checking the impact of changes it's probably fine to not. If I remember correctly a significant share of the total time is spent introspecting methods

timholy commented 4 years ago

Thanks for the great tips, @ZacLN! The next set of changes I made were orthogonal to these, so I put them in a separate PR (#160). As far as I am concerned this PR is all set, if you want it. (It probably doesn't affect performance in real-world usage all that significantly, esp. compared to #160, so whatever you choose will be fine with me.)

ZacLN commented 4 years ago

Definitely one to merge! Reductions in startup time are always worth double, right?