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

Remove antlr-rust #3

Closed
35VLG84 opened this issue Nov 16, 2024 · 8 comments · Fixed by #38
Closed

Remove antlr-rust #3

35VLG84 opened this issue Nov 16, 2024 · 8 comments · Fixed by #38
Labels
refactor Refactoring needed

Comments

@35VLG84
Copy link
Collaborator

35VLG84 commented Nov 16, 2024

Remove (or update and maintain) Antlr-Rust

The antlr-rust was great help for PoC of Tackler-NG, but it's unmaintained. Also it's not an official Antlr target. Hence remove it and replace the parser with something else.

The Gitoxide uses winnow.

@35VLG84 35VLG84 added the refactor Refactoring needed label Nov 16, 2024
@RagibHasin
Copy link

Great project! Would you accept a PR replacing antlr-rust with winnow, or are you working on it yourself?

@35VLG84
Copy link
Collaborator Author

35VLG84 commented Dec 28, 2024

Hi, I just noticed this now, after answering the #36, Welcome again, and really nice to see interest in tackler, thanks!

This is a huge offer for help, and I highly appreciate that. It would be awesome if you could provide PR for winnow parser, as I have some initial Proof-of-Concepts but nothing serious yet.

You could take look of some of the parsing related code and see if it makes sense to you:

The current ANTLR based parser is fairly well containerized behind few function calls, and there is really good test coverage for it.

But as said, this would be really big effort, and if you are not already familiar with winnow and parsing in general, this won't be easy thing to do. If that's the case, it might be better if I keep working with the parser refactoring. On the other hand, if you know winnow already, help would great and truly highly appreciated!

@35VLG84
Copy link
Collaborator Author

35VLG84 commented Dec 29, 2024

Thinking this overnight, it would be better if I replace the ANTLR as this is fairly involved part of the code base (with all tests etc). In any case, thank you for offering to port this!

@RagibHasin
Copy link

Thanks for the pointers! I would start looking into it.

@35VLG84
Copy link
Collaborator Author

35VLG84 commented Dec 30, 2024

Please do not, as I'm working on this.

@RagibHasin
Copy link

Alright.

35VLG84 added a commit that referenced this issue Jan 1, 2025
@35VLG84
Copy link
Collaborator Author

35VLG84 commented Jan 2, 2025

This is still missing some logic and special case wiring, but here is a sneak-peak of winnow port:

ANTLR

cargo bench

parser                  time:   [94.569 µs 94.721 µs 94.897 µs]"

cargo run --release -p tackler-core

On average 45903 txn/s
Reference implementation:  40000 txn/s ( +5903 txn/s)

winnow

cargo bench

parser                  time:   [2.8559 µs 2.8606 µs 2.8654 µs]

cargo run --release -p tackler-core

On average 137565 txn/s
Reference implementation:  40000 txn/s (+97565 txn/s)

It's now gix and io-bound, as the new parser is 30-35 times faster!

@RagibHasin
Copy link

That is impressive!

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

Successfully merging a pull request may close this issue.

2 participants