-
Notifications
You must be signed in to change notification settings - Fork 696
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
Is the CabalParsing
class necessary?
#10080
Comments
Yes. This is a MAJOR breaking change. If you do this, you could rip-off whole CharParsing machinery, which exists so people who don't like parsec don't need to use parsec (e.g. they could use megaparsec to get better error messages - that requires work, but currently it's possible). As an author of CharParsing I think this change, while has performance benefits, is not acceptable. |
If the problem is specialization, is it possible that |
Then fix GHC. What you do in this patch is manual optimization. If specialization is fragile, that's a GHC bug. If specialization and worker wrapper phase each other, that's also a GHC bug. Many have asked for 100% guaranteed monomorphisable abstraction (like C++ templates and Rust traits). GHC should be able to deliver. In C++ and Rust you can write code at higher abstraction level (i.e. against an interface) with guaranteed optimisation behaviour. GHC should be able to do that too. IMO 55 milliseconds per file is acceptable absolute value (IIRC this value is also skewed by few outilers, like huge files like |
Recall, Now, you propose to sweep out I don't like this trend. |
Closing following the discussion in #10081. |
Describe the feature request
The usage of the
CabalParsing
class introduces a lot of overloaded calls to the.cabal
file parser. Removing the class, and specializing references to it toParsecParser
(which, as far as I can tell, is the only instance ofCabalParsing
ever written), results in a 30% improvement in parser times and a 20% reduction in parser allocations. I have opened this PR with this change.Additional context
I am not very familiar with how the Cabal-syntax package is consumed, so the change above may not be desirable or possible. In any case, I thought I would open the PR and ask the question here, since this could be a nice performance win.
The text was updated successfully, but these errors were encountered: