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

Simplify sequential character checks in Crystal lexer #12699

Conversation

caspiano
Copy link
Contributor

A small refactor, adding char_sequence? method to check a sequence of characters.
This makes it a bit easier to read conditionals in the Crystal lexer.

Also included...

  • cleanups where a case expression is more readable than chained conditionals after an assignment in the condition.
  • some extra uses of .in?, and cleanups related to its use.

@asterite
Copy link
Member

I think I tried such an optimization in the past and it made things slightly slower. It might be worth providing a benchmark.

@caspiano
Copy link
Contributor Author

caspiano commented Oct 31, 2022

100 runs against samples/red_black_tree.cr

$ hyperfine -m 100 --warmup 5 -L type old,new -- "../bin/crystal-{type} build --no-codegen red_black_tree.cr"
Benchmark 1: ../bin/crystal-old build --no-codegen red_black_tree.cr
  Time (mean ± σ):     679.1 ms ±   5.1 ms    [User: 593.0 ms, System: 88.6 ms]
  Range (min … max):   669.0 ms … 694.8 ms    100 runs

Benchmark 2: ../bin/crystal-new build --no-codegen red_black_tree.cr
  Time (mean ± σ):     675.5 ms ±   6.8 ms    [User: 592.8 ms, System: 85.3 ms]
  Range (min … max):   652.2 ms … 686.4 ms    100 runs

Summary
  '../bin/crystal-new build --no-codegen red_black_tree.cr' ran
    1.01 ± 0.01 times faster than '../bin/crystal-old build --no-codegen red_black_tree.cr'

50 runs against all Crystal files in the samples directory produces results that are within statistical error of one another.

Command Mean [s] Min [s] Max [s] Relative
make -f Makefile.new clean all 32.817 ± 1.429 29.544 35.201 1.01 ± 0.06
make -f Makefile.old clean all 32.337 ± 1.231 29.200 33.687 1.00

@asterite I don't see a reproducible slowdown as a result of this change.

@caspiano
Copy link
Contributor Author

The worst performing and reproducible benchmark is samples/sudoku.cr.

$ hyperfine -m 100 --warmup 10 -L type new,old -- "../bin/crystal-{type} build --no-codegen sudoku.cr"
Benchmark 1: ../bin/crystal-new build --no-codegen sudoku.cr
  Time (mean ± σ):     813.6 ms ±   5.4 ms    [User: 685.2 ms, System: 134.3 ms]
  Range (min … max):   803.3 ms … 826.2 ms    100 runs

Benchmark 2: ../bin/crystal-old build --no-codegen sudoku.cr
  Time (mean ± σ):     741.6 ms ±   4.5 ms    [User: 611.4 ms, System: 136.3 ms]
  Range (min … max):   733.3 ms … 750.8 ms    100 runs

Summary
  '../bin/crystal-old build --no-codegen sudoku.cr' ran
    1.10 ± 0.01 times faster than '../bin/crystal-new build --no-codegen sudoku.cr'

Are there any oppositions to turning this helper into a macro?

Also know it's not ideal practice, but adding @[AlwaysInline] made the performance equivalent.

$ hyperfine -m 100 --warmup 10 -L type new,old -- "../bin/crystal-{type} b
uild --no-codegen sudoku.cr"
Benchmark 1: ../bin/crystal-new build --no-codegen sudoku.cr
  Time (mean ± σ):     605.1 ms ±   3.1 ms    [User: 531.9 ms, System: 76.3 ms]
  Range (min … max):   598.4 ms … 615.7 ms    100 runs

Benchmark 2: ../bin/crystal-old build --no-codegen sudoku.cr
  Time (mean ± σ):     606.2 ms ±   3.0 ms    [User: 533.0 ms, System: 76.1 ms]
  Range (min … max):   599.8 ms … 616.0 ms    100 runs

Summary
  '../bin/crystal-new build --no-codegen sudoku.cr' ran
    1.00 ± 0.01 times faster than '../bin/crystal-old build --no-codegen sudoku.cr'

@straight-shoota
Copy link
Member

You could also try making the method private. That should encourage inlining.

@asterite
Copy link
Member

I think the benchmark should just be parsing. Like using Benchmark.ips and code that just parses. I might try it later.

@asterite
Copy link
Member

I just ran a benchmark and there's no noticeable difference, so 👍

@beta-ziliani beta-ziliani added this to the 1.7.0 milestone Oct 31, 2022
@straight-shoota straight-shoota merged commit 85ef4af into crystal-lang:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants