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.

Sign up for GitHub

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

Jump to bottom

fix: make atto parser faster than the scala parser combinator one #586

Open

mbaechler wants to merge 2 commits into alonsodomin:master from mbaechler:make-atto-fast

Contributor

mbaechler commented Oct 16, 2024

For some reason, I'd rather replace the scala parser combinator implementation with something else.

I read that the atto parser is not the default one because it's slower than the current one ( #468 (comment) )

This PR fixes some correctness issues and makes it faster than the current one, it may be enough to allow the switch?

Here are my numbers regarding performances

attoParser before                                10-35 2,4,6 * ? * *  avgt   20  69.297 ± 0.223  us/op
attoParser after                                 10-35 2,4,6 * ? * *  avgt   10  43.819 ± 2.469  us/op
parserCombinators                                10-35 2,4,6 * ? * *  avgt   20  43.239 ± 0.148  us/op

attoParser before  * 5,10,15,20,25,30,35,40,45,50,55/2 * ? * mon-fri  avgt   20  82.511 ± 0.298  us/op
attoParser after   * 5,10,15,20,25,30,35,40,45,50,55/2 * ? * mon-fri  avgt   10  45.966 ± 0.409  us/op
parserCombinators  * 5,10,15,20,25,30,35,40,45,50,55/2 * ? * mon-fri  avgt   20  60.507 ± 0.212  us/op

attoParser before                                    10-65 * * * * *  avgt   20  13.337 ± 0.052  us/op
attoParser after                                     10-65 * * * * *  avgt   10   9.366 ± 0.053  us/op
parserCombinators                                    10-65 * * * * *  avgt   20  26.571 ± 0.117  us/op

attoParser before                            * */10 5-10 ? * mon-fri  avgt   20  62.982 ± 0.262  us/op
attoParser after                             * */10 5-10 ? * mon-fri  avgt   10  32.665 ± 0.265  us/op
parserCombinators                            * */10 5-10 ? * mon-fri  avgt   20  42.597 ± 0.214  us/op

attoParser before     */30 10,20,40 5-15,25-35/4 ? 1,3,7,oct-dec sun  avgt   20  38.053 ± 0.194  us/op
attoParser after      */30 10,20,40 5-15,25-35/4 ? 1,3,7,oct-dec sun  avgt   10  26.602 ± 0.175  us/op
parserCombinators     */30 10,20,40 5-15,25-35/4 ? 1,3,7,oct-dec sun  avgt   20  47.154 ± 0.181  us/op

mbaechler commented

View reviewed changes

bench/src/main/scala/cron4s/atto/package.scala

-                private val sexagesimal: Parser[Int] = int.filter(x => x >= 0 && x < 60)
-                private val decimal: Parser[Int]     = int.filter(x => x >= 0)
-                private val literal: Parser[String]  = takeWhile1(_ >= ' ')
+                private def oneOrTwoDigitsPositiveInt: Parser[Int] = {

Contributor Author

mbaechler Oct 16, 2024

this is the optimization that makes it much faster

mbaechler commented

View reviewed changes

bench/src/main/scala/cron4s/atto/package.scala Outdated


		private val sexagesimal: Parser[Int] = oneOrTwoDigitsPositiveInt.filter(x => x >= 0 && x < 60)

		private val literal: Parser[String] = takeWhile1(x => x != ' ' && x != '-')

Contributor Author

mbaechler Oct 16, 2024 •

edited

Loading

This is a correctness fix as things like mon-fri were failing previously

mbaechler commented

View reviewed changes

bench/src/main/scala/cron4s/atto/package.scala

-                    sepBy1(b, comma)
-                      .filter(_.size > 1)
-                      .map(values => SeveralNode.fromSeq[F](values.toList).get)
+                    sepBy(b, comma)

Contributor Author

mbaechler Oct 16, 2024

We were actually checking the size several times, now it's done a single time

mbaechler commented

View reviewed changes

bench/src/main/scala/cron4s/atto/package.scala Outdated

		case ParseResult.Done(_, result) => Right(result)
		case ParseResult.Fail("", _, _) => Left(ExprTooShort)

Contributor Author

mbaechler Oct 16, 2024

This is a correctness fix : a too short expression was not detected as such

mbaechler force-pushed the make-atto-fast branch from 556f6b3 to bf3f9cb Compare

October 16, 2024 16:15

mbaechler commented

View reviewed changes

bench/src/main/scala/cron4s/atto/package.scala

@@ @@ -166,8 +184,9 @@ package object atto { @@
                 } yield CronExpr(sec, min, hour, day, month, weekDay)
                 def parse(e: String): Either[Error, CronExpr] =
-                  (cron.parseOnly(e): @unchecked) match {
+                  (phrase(cron).parseOnly(e): @unchecked) match {

Contributor Author

mbaechler Oct 16, 2024

this is a correctness fix : it ensure the string is parsed entirely

mbaechler added 2 commits

October 17, 2024 12:19


          fix: the CI fails because of a specific js Date behavior

776ddb8


          fix: make atto parser faster than the scala parser combinator one

5d08ad3

mbaechler force-pushed the make-atto-fast branch from bf3f9cb to 5d08ad3 Compare

October 17, 2024 10:33

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

Labels

None yet