Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Reduce formatter's spec requires #4188

Merged
merged 1 commit into from Apr 23, 2017
Merged

Reduce formatter's spec requires #4188

merged 1 commit into from Apr 23, 2017

Conversation

makenowjust
Copy link
Contributor

@makenowjust makenowjust commented Mar 24, 2017 •
edited

./spec/compiler/formatter/formatter_spec.cr is dependent only with spec and compiler/crystal/formatter. This pull request removed spec_helper require from formatter's spec.

Now ./bin/crystal run spec/compiler/formatter/formatter_spec.cr is 7.5x faster than old.

Old:

$ rm ~/.cache/crystal
$ time ./bin/crystal run --stats spec/compiler/formatter/formatter_spec.cr
Using compiled compiler at .build/crystal
Parse:                             00:00:00.0196690 (   1.29MB)
Semantic (top level):              00:00:01.5114960 (  87.50MB)
Semantic (new):                    00:00:00.0078730 (  87.50MB)
Semantic (type declarations):      00:00:00.1385150 (  95.50MB)
Semantic (abstract def check):     00:00:00.0039040 (  95.50MB)
Semantic (ivars initializers):     00:00:00.2146060 ( 103.56MB)
Semantic (cvars initializers):     00:00:00.0149110 ( 103.56MB)
Semantic (main):                   00:00:12.6752770 ( 751.68MB)
Semantic (cleanup):                00:00:00.0025690 ( 751.68MB)
Semantic (recursive struct check): 00:00:00.0030880 ( 751.68MB)
Codegen (crystal):                 00:00:12.1741550 (1031.93MB)
Codegen (bc+obj):                  00:00:14.7028860 (1031.93MB)
Codegen (linking):                 00:00:02.4825040 (1031.93MB)

Codegen (bc+obj):
 - no previous .o files were reused
.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 169.09 milliseconds
901 examples, 0 failures, 0 errors, 0 pending
Execute:                           00:00:00.3010950 (1031.93MB)
./bin/crystal run --stats spec/compiler/formatter/formatter_spec.cr  68.42s user 4.37s system 163% cpu 44.632 total

Now:

$ rm ~/.cache/crystal
$ time ./bin/crystal run --stats spec/compiler/formatter/formatter_spec.cr
Using compiled compiler at .build/crystal
Parse:                             00:00:00.0189450 (   1.29MB)
Semantic (top level):              00:00:00.6362590 (  47.60MB)
Semantic (new):                    00:00:00.0032400 (  47.60MB)
Semantic (type declarations):      00:00:00.0618490 (  47.60MB)
Semantic (abstract def check):     00:00:00.0032140 (  47.60MB)
Semantic (ivars initializers):     00:00:00.0059740 (  47.60MB)
Semantic (cvars initializers):     00:00:00.0364030 (  47.60MB)
Semantic (main):                   00:00:01.3966960 ( 144.04MB)
Semantic (cleanup):                00:00:00.0015930 ( 144.04MB)
Semantic (recursive struct check): 00:00:00.0013040 ( 144.04MB)
Codegen (crystal):                 00:00:00.9237940 ( 176.04MB)
Codegen (bc+obj):                  00:00:02.0412920 ( 176.04MB)
Codegen (linking):                 00:00:00.1510850 ( 176.04MB)

Codegen (bc+obj):
 - no previous .o files were reused
.....................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................

Finished in 143.04 milliseconds
901 examples, 0 failures, 0 errors, 0 pending
Execute:                           00:00:00.1630650 ( 176.04MB)
./bin/crystal run --stats spec/compiler/formatter/formatter_spec.cr  9.12s user 0.72s system 175% cpu 5.616 total

I believe this fix helps accelerating crystal tool format development.

@makenowjust
Formatter: reduce spec's requires
89092e4 
`./bin/crystal run ./spec/compiler/formatter/formatter_spec.cr` is
7.5x faster than old.
@makenowjust makenowjust changed the title reduce formatter's spec requires Reduce formatter's spec requires Mar 26, 2017
@bcardiff
Copy link
Member

Maybe we should split https://github.com/crystal-lang/crystal/blob/master/spec/spec_helper.cr#L1-L3 in another file and require that at the beginning of the formatter_spec.cr. That will also allow us to avoid the relative require of the formatter code.

WDYT?

@makenowjust makenowjust closed this Apr 8, 2017
@makenowjust makenowjust reopened this Apr 8, 2017
@makenowjust
Copy link
Contributor Author

Closing pull request is just mistake. Sorry 😢

@makenowjust
Copy link
Contributor Author

@bcardiff Totally good. I think we don't need discussion about it. But this pull request is self-contained. I think your suggestion can be separated from this pull request.

@miketheman
Copy link
Contributor

#codetriage

We last left this with the open question from @bcardiff about making another change to the spec_helper.cr file and @makenowjust responded with the detail that this PR seems to be self-contained and solves this.

I think that "relative requires" are part of the current game, until such a time where Crystal does auto-require-path-search, so I don't think there's anything inherently wrong with it.

@mverzilli
Copy link

Thanks @miketheman, I agree with your asessment. Let's merge this, the compile time gains are huge and the change is pretty hygienic. 7.5x better compile times just by being more specific declaring dependencies? Where do I sign? :).

@makenowjust this is an amazing contribution, thanks so much ❤️.

@mverzilli mverzilli merged commit 65a3f5a into crystal-lang:master Apr 23, 2017
@mverzilli mverzilli added this to the Next milestone Apr 23, 2017
@makenowjust makenowjust deleted the fix/crystal-format/spec-reduce-dependency branch April 24, 2017 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
kind:refactor topic:tools:formatter
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@makenowjust @bcardiff @miketheman @mverzilli