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

Parser optimizations #537

Merged
merged 15 commits into from
Mar 3, 2022
Merged

Conversation

asterite
Copy link
Contributor

Hi!

See discussion starting at #536 (comment)

This is just a draft... I think the style or final code can be further improved, but at least the optimization ideas should be clear from the commits.

This is the benchmark I used:

require "./src/all"
require "benchmark"

filename = "icons.mint"
source = File.read(filename)

Benchmark.ips do |x|
  x.report("parse") do
    Mint::Parser.parse(source, filename)
  end
end

The file "icons.mint" comes from here: https://github.com/mint-lang/mint-tabler-icons/blob/as-functions/source/Icons.mint

Before this PR:

parse 108.63m (  9.21s ) (± 0.00%)  7.33GB/op  fastest

With this PR:

parse  68.05  ( 14.70ms) (± 3.80%)  20.1MB/op  fastest

@gdotdesign
Copy link
Member

Thank you very much 🙏 I totally missed that raise, we have run into this before 🤦 I'll take a look at other optimizations you made to learn from it :)

@asterite
Copy link
Contributor Author

Also with the previous benchmark:

require "./src/all"
require "benchmark"

filename = "./core/source/Provider/ElementSize.mint"
source = File.read(filename)
Benchmark.ips do |x|
  x.report("parse") do
    Mint::Parser.parse(source, filename)
  end
end

Before:

parse   8.55k (117.00µs) (± 4.06%)  83.9kB/op  fastest

After:

parse  15.92k ( 62.82µs) (± 6.46%)  37.2kB/op  fastest

So it's about twice as fast and consumes half the memory in the general case.

And for this particular icons.mint file, the speedup i like 600x faster and like 360 less memory consumed.

I still have to investigate why it takes 20MB to parse that file, so there's still room for optimization! But it can be done in a separate PR.

@asterite asterite marked this pull request as ready for review February 25, 2022 09:48
src/parser.cr Show resolved Hide resolved
src/parser.cr Outdated Show resolved Hide resolved
src/parser.cr Outdated Show resolved Hide resolved
src/parser.cr Outdated Show resolved Hide resolved
@gdotdesign gdotdesign requested a review from Sija March 3, 2022 09:26
@gdotdesign gdotdesign added enhancement New feature or request refactor labels Mar 3, 2022
src/parser.cr Outdated Show resolved Hide resolved
src/parser.cr Outdated Show resolved Hide resolved
src/parser.cr Outdated Show resolved Hide resolved
asterite and others added 2 commits March 3, 2022 08:18
Co-authored-by: Sijawusz Pur Rahnama <sija@sija.pl>
Copy link
Member

@Sija Sija left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 (missing spec for next_whitespace_index would be a nice to have, but not required)

@gdotdesign gdotdesign merged commit 176a508 into mint-lang:master Mar 3, 2022
@gdotdesign
Copy link
Member

Thank you very much! 🙏 🎉

@Sija Sija added this to the 0.16.0 milestone Mar 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request refactor
Development

Successfully merging this pull request may close these issues.

3 participants