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

Use Ragel to Parse the Version Information #11

Closed
wants to merge 6 commits into from
Closed

Conversation

pope
Copy link
Contributor

@pope pope commented Oct 25, 2016

I was able to eke out the slightest bit more performance by using Ragel to do the parsing. I offer it back if using another tool doesn't solve more problems than it causes.

I also fixed a few warnings while I was in there.

Merge in latest changes
The benefit is that it runs faster and in theory it should be easy to
add new version rules.
The capacity was off by 1 because there's already 1 `union version_comp`
block of memory in the sizeof calculation for `struct version_number`.
@mislav
Copy link
Contributor

mislav commented Oct 25, 2016

Interesting! But if it's only a "slightest bit" more performance, I feel that it's not worth risking a huge change. Version sorting isn't a performance problem right now on GitHub.com like when it was when it was implemented in Ruby.

Do you mind sharing the script/benchmark output?

@mislav
Copy link
Contributor

mislav commented Oct 25, 2016

Also, both files now say “Created with Ragel.” This can't be true, as one file is definitely generated, and one file is Ragel source. Which is which?

@pope
Copy link
Contributor Author

pope commented Oct 25, 2016

Yeah, totally agree that this isn't something you need. I was just porting over some version parsing work that I did elsewhere with a similar ragel machine and saw that it sped this up.

After I re-record the benchmarks locally.

VersionSorter .sort
  Baseline: |                                                              X---|
  Current:  |                                                           X----  |
            0                                                         488.953 us

VersionSorter .rsort
  Baseline: |                                                             X--  |
  Current:  |                                                          X-------|
            0                                                         509.559 us

I agree with the double "Created with Ragel" stuff. Would you accept a README file in that directory?

@mislav
Copy link
Contributor

mislav commented Oct 26, 2016

Thanks for sharing the benchmark. You were right that the speedup is only minor. I don't think it warrants the significant change to the codebase. Thank you for suggesting, though!

@mislav mislav closed this Oct 26, 2016
@pope
Copy link
Contributor Author

pope commented Oct 26, 2016

Yup. Thanks for considering. I get it that the risk/reward isn't great, but thought I would share it back. Cheers.

@mislav
Copy link
Contributor

mislav commented Oct 27, 2016

It's always interesting to see neat uses for Ragel. One other project that I found really interesting was a vim keystroke parser written in Ragel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants