Improves performance when analyzing on save. #12
Conversation
src/scry/analyzer.cr
Outdated
| @@ -5,12 +5,22 @@ require "./publish_diagnostic" | |||
|
|
|||
| module Scry | |||
| struct Analyzer | |||
| @@source = [] of String | |||
There was a problem hiding this comment.
I've been out of the Scry codebase for a while, but I'm a bit concerned about this being set as a class var. Doesn't this make it impossible to check a project with multiple files against a single running server instance?
There was a problem hiding this comment.
In VSCode, every workspace launch a new instance of scry, in NVIM too. I don't know in Atom, however if the code inside text_document.text changes, then the source file is updated.
My goals with this variable is to check that source is different when document is saved, because the actual behavior is checking the file too much, spending memory and CPU time even when code is the same.
It's a class variable because a new instance of Analyzer is created in a new message, and I didn't have other way to handle this.
There was a problem hiding this comment.
We can leave this for now, but this seems like a likely place for a race condition. It may be time to consider re-architecting the Analyzer.
src/scry/analyzer.cr
Outdated
| if @@source != @text_document.text | ||
| @@source = @text_document.text | ||
| @@source.map do |text| | ||
| unless @text_document.filename.starts_with?("/usr/lib") || |
There was a problem hiding this comment.
Is this a global shard home directory? Is there an env var we could use instead?
There was a problem hiding this comment.
Yes I can use, ENV["CRYSTAL_PATH"]
Let me fix that, I will do on weekend.
src/scry/implementations.cr
Outdated
| @@ -39,6 +39,10 @@ module Scry | |||
| end | |||
| ResponseMessage.new(@text_document.id, locations) | |||
| end | |||
| rescue ex | |||
| nil | |||
There was a problem hiding this comment.
Should we at least log that an exception happened? What kind of exceptions are we expecting here?
There was a problem hiding this comment.
Yes something like, Log.logger.error("A error was found while searching definitions\n#{ex}")
|
@kofno Thanks you for review my code 😅 I will improve this PR on weekend. The aims of this PR is to reduce scry resources, because currently scry analyzer take up to 500 Mb with a simple file. Without analyzer scry uses only 10Mb in my computer to do formatting, syntax checking and goto defitiniton. I know that isn't a scry problem but a crystal problem, but at least we can do something for now. Cheers |
|
Yes, I think the same, Maybe the analyzer could be implemented using spawn
and channels.
No problem with leaving it out, I'll change this in the next weeks.
…On Wed, Jul 12, 2017 at 6:43 AM, Ryan L. Bell ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/scry/analyzer.cr
<#12 (comment)>:
> @@ -5,12 +5,22 @@ require "./publish_diagnostic"
module Scry
struct Analyzer
+ @@source = [] of String
We can leave this for now, but this seems like a likely place for a race
condition. It may be time to consider re-architecting the Analyzer.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#12 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AC7Nx2JcTReExqdmGwyDQWeKYj7Q3CdPks5sNLFwgaJpZM4OSqXB>
.
|
|
I want to read of the code for some of the the other language servers, just to get some ideas before we get too deep into refactoring. I'd also like to read some general Crystal and Go server code to get a good feel for using channels again. I worked through some examples before, but I need a refresher I think. |
|
Hi @kofno 😄 I removed |
|
Done! Thanks again! |
Hi @kofno I did some improves.
Verify that document text is different before analyze.