-
-
Notifications
You must be signed in to change notification settings - Fork 251
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
change fast-parse to cat-parse #834
Conversation
Ouch, it's very slow 😢
In both cases it would make caliban much slower than sangria. Now, do you think the parsing code could be optimized, or is the slowness entirely caused by cats-parse? If I get some time this weekend I could do some profiling to see what makes it so slow. PS: run |
I just make def to val, and remove P.defer0, It improve a lot. val document: Parser0[ParsedDocument] =
(P.start *> whitespaceWithComment *> definition.repSep0(whitespaceWithComment) <* whitespaceWithComment <* P.end).map(seq => ParsedDocument(seq))
I will change it tomorrow. |
The performance does not change so much. Last night benchmark result can not reach today with same code, don't know the reason. |
@ghostdogpr It may be the final version. I don't know if there have other ways to improve performance |
I'll dig into cats-parse and see if the profiler shows something obvious to improve, maybe this weekend or the next. |
@timzaak I did some profiling of your code. I found 2 things:
Btw I will review the correctness more in details once the performance issue is solved. |
@ghostdogpr thanks for your profile. I will watch typelevel/cats-parse#198 |
wait until cats-parse 0.3.3 release |
@timzaak can you rebase your PR into this branch? #847 This is the branch I created for Scala 3 support. The code that is specific to Scala 2 is in the |
@ghostdogpr I will do it later |
ref #650 lots of code are from lvitaly.
I have run benchmarks on my desktop computer.
the cat-parse result:
the fast-parse result:
don't know if it's fast enough to replace fast-parse