d-language-server / dls

A Language Server implementation for D
http://dls.dub.pm
106 stars 15 forks source link

#44 Improve startup time for large workspaces. #46

Closed ghost closed 5 years ago

ghost commented 5 years ago

The logging was the slowest part, opening and closing a file constantly 7000 times is slow. Not sure how useful it is for you, but I just removed the logging for now with some other changes. It only takes about a second or two to startup now. Could probably still be improved a bit, but it is a lot faster now.

LaurentTreguier commented 5 years ago

Logging should usually be disabled by default; if not, the editor should give an option to control them (like d.trace.server in VSCode for example). If you don't need the logs, then you should simply not set a log file, and DLS should just not log anything on disk (am I misunderstanding your logging usage ?) You did make me notice that file logging didn't respect the log level set by the client though, and file logs are getting all possible logs, so I'm preparing a quick release to at least fix that. d.trace.server will be honored for file logging now.

Regarding the other changes, did they improve performance much ? They didn't seem to improve much from my quick tests. I didn't try to see the changes with logging however, that doesn't need testing to know it could easily destroy performance.

ghost commented 5 years ago

Yah, just iterating the files was taking a while. It was constructing a new array of Document.uri for every file in the workspace. It was also calling isFile on path which would ping the filesystem a second time. That information was already obtained from dirEntries. It was also constructing the URI which would do the regex which was rather slow. Profiling it was taking up a good chunk of time when it wasn't needed.

Logging is useful for debugging, just the why it needs to be debugged at least. Maybe there's an easier way, but logging relevant data is useful. I don't find those messages all that useful, and when trying to log they make start up take a minute or so it can be annoying.

LaurentTreguier commented 5 years ago

What did you use to profile it ? I tried building with dub build --build=profile but it causes an "Unreachable statement" error in DCD. From my basic time measurements, I didn't notice any speed boost from those changes. I made minor changes to not unnecessarily call isFile and make new arrays, and to create Uris manually. However, _uri is not set as a constructor parameter but re-created from the other parts. I'm not fond of the idea of letting the caller potentially create inconsistent Uris.

For logging, in VSCode you can now simply set d.trace.server to messages and it will omit the logs you are removing in this PR.

ghost commented 5 years ago
            workspacesUris.map!(uri => dirEntries(uri.path, SpanMode.depth)
                .filter!q{a.isFile}
                .map!q{a.name}
                .filter!(file => globMatch(file, "*.{d,di}"))
                .map!(Uri.fromPath)
                .filter!(uri => !Document.uris.canFind!sameFile(uri)) // <-----
                .filter!isImported
                .array));

This is going to be allocating new URIs for each file as well. Document.uri is a mapped range that converts it to a new Uri. Don't really like chained commands this long, you get to he point where you are creating so many templates and prompting the GC to create contexts where you need it but is easily avoidable otherwise, that's just my preference.

I'm using Visual Studio and Intel VTune to profile it. It depends on the project I guess, not sure which project you are testing with but it does reduce it by at least 1 second for me. I guess it would depend on your computer as well, mine is a bit old.

ghost commented 5 years ago

The purpose of this change is for sockets. There is no concept of a "flush", or rather I don't know if the D api exposes it. It is roughly half as fast if you don't send 1 character packets. Either way sending the entire message at once is the better option.

// socket before
scanAllWorkspaces took: 1 sec, 896 ms, and 615 μs
scanAllWorkspaces took: 1 sec, 923 ms, and 251 μs
scanAllWorkspaces took: 1 sec, 687 ms, 518 μs, and 6 hnsecs
scanAllWorkspaces took: 1 sec, 776 ms, 226 μs, and 7 hnsecs

// socket after 
scanAllWorkspaces took: 854 ms, 46 μs, and 1 hnsec
scanAllWorkspaces took: 1 sec, 91 ms, 433 μs, and 5 hnsecs
scanAllWorkspaces took: 941 ms, 668 μs, and 9 hnsecs
scanAllWorkspaces took: 895 ms, 668 μs, and 6 hnsecs
scanAllWorkspaces took: 911 ms, 284 μs, and 7 hnsecs

No real difference for stdio since it can just flush, if not it is a bit more consistence since it might flush on its own.

LaurentTreguier commented 5 years ago

Makes sense, I didn't think about sockets looking at this bit. Can you change the PR to be against the release/v0.25.x branch ? I use master to deliver new minor versions with notable changes, and since I'm not actively working on DLS at the moment, it would be a very long time before a new minor version comes out.