Make scry faster and lightweight #53

faustinoaq · 2018-03-04T13:46:25Z

Hi, this PR implements lightweight features:

Lightweight analyzer and implementations
Avoid heavy compiler dependencies
Use external process to free up memory

Before:

With this PR changes:

Same features and behaviors, more RAM friendly and lightweight binary 19.2MB vs 4.7MB

Oh, also compilation time has been reduced drastically:

$ time crystal build src/scry.cr -s -p --no-debug
Parse:                             00:00:00.000316202 (   0.25MB)
Semantic (top level):              00:00:00.404959721 (  52.33MB)
Semantic (new):                    00:00:00.002962556 (  52.33MB)
Semantic (type declarations):      00:00:00.032710793 (  52.33MB)
Semantic (abstract def check):     00:00:00.002463025 (  52.33MB)
Semantic (ivars initializers):     00:00:00.001851865 (  52.33MB)
Semantic (cvars initializers):     00:00:00.240542928 (  68.33MB)
Semantic (main):                   00:00:01.663311243 ( 196.77MB)
Semantic (cleanup):                00:00:00.006314623 ( 196.77MB)
Semantic (recursive struct check): 00:00:00.002034422 ( 196.77MB)
Codegen (crystal):                 00:00:01.197870686 ( 228.77MB)
Codegen (bc+obj):                  00:00:05.765841086 ( 228.77MB)
Codegen (linking):                 00:00:00.761140190 ( 228.77MB)
                                          
Codegen (bc+obj):
 - no previous .o files were reused
crystal build src/scry.cr -s -p --no-debug  14.42s user 0.81s system 149% cpu 10.159 total

Previously scry compilation used a huge amout of RAM (above 1GB) and took several minutes to compile in my pc 😓

Fixes #30

Related comment on #48 (comment)

- Lightweight analyzer and implementations - Avoid heavy compiler dependencies - Use external process to freeup memory

bew · 2018-03-04T16:00:48Z

I wonder how the completion system will work with this

faustinoaq · 2018-03-04T16:18:42Z

I wonder how the completion system will work with this

@bew auto-completion feature will keep working as usual

laginha87 · 2018-03-04T20:31:24Z

Completion just uses the compiler/syntax from crystal to build the ast trees.
Glad to see the reduction in compilation time.
I don't how I feel about passing it off to the crystal tools though, seems like it might create a bigger dependency on what version of crystal the user is using.

faustinoaq · 2018-03-05T00:26:58Z

Completion just uses the compiler/syntax from crystal to build the ast trees. Glad to see the reduction in compilation time.

👍

I don't how I feel about passing it off to the crystal tools though, seems like it might create a bigger dependency on what version of crystal the user is using.

@laginha87 Then, a developer using scry will see the errors and implementation behaviors according to the crystal version that he/she is using. Will not depend on the crystal version that scry was compiled, so finally, is another benefit for using an external crystal process 😄

keplersj · 2018-03-05T00:33:30Z

Like I said here I'm still not sure this is a good idea.

If this PR included README changes stating that this is dependent on the user having the Crystal compiler installed in their PATH and included a mechanism to alert the user if they don't when the server starts I would be more onboard with a change like this.

faustinoaq · 2018-03-05T00:51:15Z

If this PR included README changes stating that this is dependent on the user having the Crystal compiler installed in their PATH and included a mechanism to alert the user if they don't when the server starts I would be more onboard with a change like this.

Hi @keplersj 😄

We're already dependent on crystal command installed and avaliable on PATH. Scry is already executing crystal here:

scry/src/scry/environment_config.cr

Line 25 in 1bff42e

Process.run("crystal", ["env"], output: io)

So currently if a user doesn't have crystal installed, then scry won't work

This PR just take advantage of using crystal command to be memory friendly, to compile faster and to generate a smaller binary.

keplersj · 2018-03-05T00:57:25Z

@faustinoaq Hmm... Okay. I still can't shake this feeling that relying on the Crystal compiler command line is going to be a bad idea in the long run and probably breaks with the norms of other LSP servers. But, you have good points. I'm okay with this for now.

I do want to hear @bcardiff's opinion on this though.

faustinoaq · 2018-03-05T01:09:13Z

I still can't shake this feeling that relying on the Crystal compiler command line is going to be a bad idea in the long run and probably breaks with the norms of other LSP servers.

The src/protocol folder will work as usual, this PR just modifies the way to process 2 heavy features (diagnostics and implementations) and keeps other things untouched.

In fact the only disadvantage of using this approach is that compiler could output extra characters to stdio in some special situations: like Using bin/crystal at commit kjdf6zx (can be disabled) commonly shown in local crystal builds from git repo or benchmark warning like Using benchmark without --release flag, bla, bla... or some crystal bug. because it will break json output parsing.

bmulvihill · 2018-03-05T13:40:58Z

@faustinoaq How will this affect CI? We should be able to remove any LLVM dev dependencies?

faustinoaq · 2018-03-05T18:50:05Z

How will this affect CI? We should be able to remove any LLVM dev dependencies?>

@bmulvihill Oh yeah, another advantage of this PR, LLVM won't be a requirement to compile scry! 🌮

➜  scry git:(fa/lightweight-scry-speedup) ✗ sudo pacman -Rns llvm
[sudo] password for main: 
checking dependencies...

Packages (1) llvm-5.0.1-2

Total Removed Size:  159.96 MiB

:: Do you want to remove these packages? [Y/n] Y
:: Processing package changes...
(1/1) removing llvm               [#####################################################] 100%
:: Running post-transaction hooks...
(1/1) Arming ConditionNeedsUpdate...
➜  scry git:(fa/lightweight-scry-speedup) ✗ time crystal build src/scry.cr -s -p --no-debug
Parse:                             00:00:00.000311398 (   0.25MB)
Semantic (top level):              00:00:00.379528606 (  52.33MB)
Semantic (new):                    00:00:00.002840074 (  52.33MB)
Semantic (type declarations):      00:00:00.034088660 (  52.33MB)
Semantic (abstract def check):     00:00:00.002330909 (  52.33MB)
Semantic (ivars initializers):     00:00:00.001950688 (  52.33MB)
Semantic (cvars initializers):     00:00:00.250472900 (  68.33MB)
Semantic (main):                   00:00:01.612165740 ( 196.77MB)
Semantic (cleanup):                00:00:00.006179480 ( 196.77MB)
Semantic (recursive struct check): 00:00:00.001546292 ( 196.77MB)
Codegen (crystal):                 00:00:01.210651356 ( 228.77MB)
Codegen (bc+obj):                  00:00:00.387764860 ( 228.77MB)
Codegen (linking):                 00:00:00.714893504 ( 228.77MB)
                                          
Codegen (bc+obj):
 - all previous .o files were reused
crystal build src/scry.cr -s -p --no-debug  4.80s user 0.47s system 112% cpu 4.674 total
➜  scry git:(fa/lightweight-scry-speedup) ✗ rm -rf ~/.cache/crystal                        
➜  scry git:(fa/lightweight-scry-speedup) ✗ time crystal build src/scry.cr -s -p --no-debug
Parse:                             00:00:00.000323728 (   0.25MB)
Semantic (top level):              00:00:00.395329747 (  52.33MB)
Semantic (new):                    00:00:00.002867334 (  52.33MB)
Semantic (type declarations):      00:00:00.036601810 (  52.33MB)
Semantic (abstract def check):     00:00:00.002567500 (  52.33MB)
Semantic (ivars initializers):     00:00:00.002118485 (  52.33MB)
Semantic (cvars initializers):     00:00:00.246402066 (  68.33MB)
Semantic (main):                   00:00:01.628563404 ( 196.77MB)
Semantic (cleanup):                00:00:00.005707169 ( 196.77MB)
Semantic (recursive struct check): 00:00:00.001652108 ( 196.77MB)
Codegen (crystal):                 00:00:01.205903900 ( 228.77MB)
Codegen (bc+obj):                  00:00:06.013242583 ( 228.77MB)
Codegen (linking):                 00:00:00.712104028 ( 228.77MB)
                                          
Codegen (bc+obj):
 - no previous .o files were reused
crystal build src/scry.cr -s -p --no-debug  14.66s user 0.71s system 148% cpu 10.323 total

😄

faustinoaq · 2018-03-05T19:17:18Z

How will this affect CI? We should be able to remove any LLVM dev dependencies?

Well, now CI is 3X faster 🚀

Before: 5.13s

Now 1.44s 💯

Memory usage has changed as well, 1GB vs 250MB 😅

bmulvihill

@faustinoaq Originally I was against this idea, but I think the benefit of not needing to package the entire Crystal compiler is a big win.

keplersj · 2018-03-05T20:49:37Z

@bmulvihill I second your comments. I've been swayed. This is a good idea. If there were a way to interact with the compiler programmatically without all the weight I'd be for that, but this works well in this form.

faustinoaq · 2018-03-05T21:26:53Z

So, I think this is ready, WDYT about merging this?

keplersj · 2018-03-05T21:29:18Z

faustinoaq added 2 commits March 4, 2018 08:31

Fix typo

baa6d4b

Implements lightweight features

7019371

- Lightweight analyzer and implementations - Avoid heavy compiler dependencies - Use external process to freeup memory

faustinoaq requested a review from a team March 4, 2018 13:46

Add --no-codegen flag

c41df7b

keplersj approved these changes Mar 5, 2018

View reviewed changes

faustinoaq added 5 commits March 5, 2018 13:51

Remove LLVM from README

1bc2ab8

Revome extra space

69f998a

Remove LLVM dependency

e3d9c8b

Remove LLVM dependency on OSX

3d8f296

Merge branch 'master' into fa/lightweight-scry-speedup

0b32d3f

bmulvihill approved these changes Mar 5, 2018

View reviewed changes

faustinoaq merged commit 8653d63 into master Mar 6, 2018

faustinoaq deleted the fa/lightweight-scry-speedup branch March 6, 2018 00:35

faustinoaq mentioned this pull request Apr 6, 2018

Remove Scry from deploys crystal-lang-tools/crystal-ide#12

Closed

straight-shoota mentioned this pull request Apr 16, 2018

Crystal source formatting rule crystal-ameba/ameba#49

Closed

faustinoaq mentioned this pull request May 6, 2018

Support find references #104

Closed

bmulvihill mentioned this pull request Apr 18, 2019

Building gives undefined constant Crystal::Codegen::Target error #155

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make scry faster and lightweight #53

Make scry faster and lightweight #53

faustinoaq commented Mar 4, 2018 •

edited

Loading

bew commented Mar 4, 2018

faustinoaq commented Mar 4, 2018 •

edited

Loading

laginha87 commented Mar 4, 2018

faustinoaq commented Mar 5, 2018

keplersj commented Mar 5, 2018 •

edited

Loading

faustinoaq commented Mar 5, 2018

keplersj commented Mar 5, 2018

faustinoaq commented Mar 5, 2018 •

edited

Loading

bmulvihill commented Mar 5, 2018 •

edited

Loading

faustinoaq commented Mar 5, 2018

faustinoaq commented Mar 5, 2018

bmulvihill left a comment

keplersj commented Mar 5, 2018

faustinoaq commented Mar 5, 2018

keplersj commented Mar 5, 2018

Make scry faster and lightweight #53

Make scry faster and lightweight #53

Conversation

faustinoaq commented Mar 4, 2018 • edited Loading

bew commented Mar 4, 2018

faustinoaq commented Mar 4, 2018 • edited Loading

laginha87 commented Mar 4, 2018

faustinoaq commented Mar 5, 2018

keplersj commented Mar 5, 2018 • edited Loading

faustinoaq commented Mar 5, 2018

keplersj commented Mar 5, 2018

faustinoaq commented Mar 5, 2018 • edited Loading

bmulvihill commented Mar 5, 2018 • edited Loading

faustinoaq commented Mar 5, 2018

faustinoaq commented Mar 5, 2018

bmulvihill left a comment

Choose a reason for hiding this comment

keplersj commented Mar 5, 2018

faustinoaq commented Mar 5, 2018

keplersj commented Mar 5, 2018

faustinoaq commented Mar 4, 2018 •

edited

Loading

faustinoaq commented Mar 4, 2018 •

edited

Loading

keplersj commented Mar 5, 2018 •

edited

Loading

faustinoaq commented Mar 5, 2018 •

edited

Loading

bmulvihill commented Mar 5, 2018 •

edited

Loading