-
Notifications
You must be signed in to change notification settings - Fork 121
Friends,
last time I profiled Packwerk a few years ago, about 2/3 of the execution time was spent parsing. A coworker just recently profiled it again and basically this still seems to be the case.
I tried to write a new parser based on ripper in 2021, when I was still at Shopify. However, that turned out to be a lot of work. Back then, I was hoping for @kddnewton 's work on a new parser to bear fruit, so that we can use it for packwerk at some point.
Well, prism is now a thing. Would it make sense to swap out the currently used whitequark parser for prism?
@kddnewton how much faster do you expect Prism to be compared with the whitequark parser? I couldn't find any benchmarks.
If this makes sense, I might take a stab at it - provided I can get someone to pay me for the time spent on it
All reactions
I would expect it to be significantly faster. As an example, I used it as the basis of the parser gem for running rubocop, and the benchmarks are here: https://github.com/kddnewton/parser-prism?tab=readme-ov-file#benchmarks.
I would be very supportive of this work!
Replies: 2 comments 5 replies
I would expect it to be significantly faster. As an example, I used it as the basis of the parser gem for running rubocop, and the benchmarks are here: https://github.com/kddnewton/parser-prism?tab=readme-ov-file#benchmarks.
I would be very supportive of this work!
All reactions
Is parser-prism production ready or are you pllaning to get it there?
If we could just swap that in without having to rewrite the code currently specific to whitequark/parser, that sounds like a good option
All reactions
I tried parser-prism and I only get a single failing test #388
I'll look into that one
All reactions
The problem seems to be that Parser::Prism raises a StandardError here, whereas Parser::CurrentRuby raises a Parser::SyntaxError in that case.
Parser::Prism raises a more specific error on parser-prism's main branch. We could cath that one explicitly. However, it seems for compatibility you may want to raise the same exceptions as Parser::CurrentRuby?
All reactions
Let's continue the discussion on the PR
All reactions
I believe we've discussed this internally at some point. I'm also very much in favour of this change, but we would have to update NodeHelpers among other things that are keyed to the parser api.
All reactions
It looks like we might not have to if we use parser-prism? See comment above.