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

Is the CabalParsing class necessary? #10080

Closed
FinleyMcIlwaine opened this issue Jun 7, 2024 · 5 comments
Closed

Is the CabalParsing class necessary? #10080

FinleyMcIlwaine opened this issue Jun 7, 2024 · 5 comments

Comments

@FinleyMcIlwaine
Copy link
Contributor

FinleyMcIlwaine commented Jun 7, 2024

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 to ParsecParser (which, as far as I can tell, is the only instance of CabalParsing 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.

@phadej
Copy link
Collaborator

phadej commented Jun 13, 2024

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.

@tbidne
Copy link
Contributor

tbidne commented Jun 13, 2024

If the problem is specialization, is it possible that INLINEABLE may yield performance wins? I.e. adding {-# INLINEABLE foo #-} to every function that with a signature like foo :: CabalParsing m => m Int (I realize there are many).

@phadej
Copy link
Collaborator

phadej commented Jun 16, 2024

This patch demonstrates how you could begin adding SPECIALIZE pragmas to remove some of the overloading. However, the performance improvements aren't as good ... and the specialization is somewhat fragile, since in some cases we need to delay inlining to ensure the rewrite rules can fire before worker wrapper.

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 acme-everything; it would be better to compare medians). If there is an abstraction cost, it's fine. Hopefully GHC will do better in the future.

@phadej
Copy link
Collaborator

phadej commented Jun 16, 2024

Recall, Generic LicenseId was also removed because GHC is "too slow" to compile it. And no-one is looking at making GHC.Generics viable for large types, partly because the problem is swept under the rug.

Now, you propose to sweep out CabalParsing, so specialization issues aren't right there for GHC developers to work on.

I don't like this trend.

@FinleyMcIlwaine
Copy link
Contributor Author

Closing following the discussion in #10081.

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

No branches or pull requests

3 participants