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.

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

Defer creation of Expectations #280

Merged
merged 6 commits into from
Oct 29, 2021
Merged

Defer creation of Expectations #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea427b9
Defer creation of expectations
rossabaker Oct 15, 2021
7de4372
scalafmt
rossabaker Oct 15, 2021
dedd158
s/Eval/thunks/g
rossabaker Oct 15, 2021
c52a6bb
Revert "s/Eval/thunks/g" -- not stack safe
rossabaker Oct 15, 2021
bbfe4d7
Add MiMa filter for CharIn.makeError
rossabaker Oct 15, 2021
9561249
Restore optimization from #62
rossabaker Oct 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import com.typesafe.tools.mima.core._
import sbtcrossproject.CrossPlugin.autoImport.{crossProject, CrossType}
import Dependencies._

Expand Down Expand Up @@ -133,7 +134,12 @@ lazy val core = crossProject(JSPlatform, JVMPlatform)
mimaPreviousArtifacts := {
val isScala211 = CrossVersion.partialVersion(scalaVersion.value).contains((2, 11))
if (isScala211) Set.empty else mimaPreviousArtifacts.value
}
},
mimaBinaryIssueFilters := Seq(
ProblemFilters.exclude[IncompatibleResultTypeProblem](
"cats.parse.Parser#Impl#CharIn.makeError"
) // Impl is private to cats.parse.Parser
)
)
.jsSettings(
crossScalaVersions := (ThisBuild / crossScalaVersions).value.filterNot(_.startsWith("2.11")),
Expand Down
72 changes: 45 additions & 27 deletions core/shared/src/main/scala/cats/parse/Parser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ sealed abstract class Parser0[+A] { self: Product =>
val offset = state.offset
if (err eq null) Right((str.substring(offset), result))
else
Left(Parser.Error(offset, Parser.Expectation.unify(NonEmptyList.fromListUnsafe(err.toList))))
Left(
Parser.Error(
offset,
Parser.Expectation.unify(NonEmptyList.fromListUnsafe(err.value.toList))
)
)
}

/** Attempt to parse all of the input `str` into an `A` value.
Expand All @@ -92,7 +97,12 @@ sealed abstract class Parser0[+A] { self: Product =>
)
)
} else
Left(Parser.Error(offset, Parser.Expectation.unify(NonEmptyList.fromListUnsafe(err.toList))))
Left(
Parser.Error(
offset,
Parser.Expectation.unify(NonEmptyList.fromListUnsafe(err.value.toList))
)
)
}

/** Convert epsilon failures into None values.
Expand Down Expand Up @@ -1773,7 +1783,7 @@ object Parser {
*/
protected[parse] final class State(val str: String) {
var offset: Int = 0
var error: Chain[Expectation] = null
var error: Eval[Chain[Expectation]] = null
var capture: Boolean = true
}

Expand Down Expand Up @@ -2033,7 +2043,8 @@ object Parser {
state.offset = end
res
} else {
state.error = Chain.one(Expectation.Length(offset, len, state.str.length - offset))
state.error =
Eval.later(Chain.one(Expectation.Length(offset, len, state.str.length - offset)))
null
}
}
Expand Down Expand Up @@ -2079,17 +2090,19 @@ object Parser {

case object StartParser extends Parser0[Unit] {
override def parseMut(state: State): Unit = {
if (state.offset != 0) {
state.error = Chain.one(Expectation.StartOfString(state.offset))
val offset = state.offset
if (offset != 0) {
state.error = Eval.later(Chain.one(Expectation.StartOfString(offset)))
}
()
}
}

case object EndParser extends Parser0[Unit] {
override def parseMut(state: State): Unit = {
if (state.offset != state.str.length) {
state.error = Chain.one(Expectation.EndOfString(state.offset, state.str.length))
val offset = state.offset
if (offset != state.str.length) {
state.error = Eval.later(Chain.one(Expectation.EndOfString(offset, state.str.length)))
}
()
}
Expand Down Expand Up @@ -2128,7 +2141,7 @@ object Parser {
state.offset += message.length
()
} else {
state.error = Chain.one(Expectation.OneOfStr(offset, message :: Nil))
state.error = Eval.later(Chain.one(Expectation.OneOfStr(offset, message :: Nil)))
()
}
}
Expand All @@ -2144,29 +2157,31 @@ object Parser {
state.offset += message.length
()
} else {
state.error = Chain.one(Expectation.OneOfStr(offset, message :: Nil))
state.error = Eval.later(Chain.one(Expectation.OneOfStr(offset, message :: Nil)))
()
}
}
}

case class Fail[A]() extends Parser[A] {
override def parseMut(state: State): A = {
state.error = Chain.one(Expectation.Fail(state.offset));
val offset = state.offset
state.error = Eval.later(Chain.one(Expectation.Fail(offset)))
null.asInstanceOf[A]
}
}

case class FailWith[A](message: String) extends Parser[A] {
override def parseMut(state: State): A = {
state.error = Chain.one(Expectation.FailWith(state.offset, message));
val offset = state.offset
state.error = Eval.later(Chain.one(Expectation.FailWith(offset, message)))
null.asInstanceOf[A]
}
}

final def oneOf[A](all: Array[Parser0[A]], state: State): A = {
val offset = state.offset
var errs: Chain[Expectation] = Chain.nil
var errs: Eval[Chain[Expectation]] = Eval.later(Chain.nil)
Copy link
Collaborator

@johnynek johnynek Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make this Eval.now(Chain.nil) I think that will be less allocation and actually every oneOf hits this path.

actually, can we allocate a single Eval[Chain[Expectation]] as a private val evalEmpty: Eval[Chain[Expectation]] = Eval.now(Chain.nil) in Impl and not have any allocations for this?

var idx = 0
while (idx < all.length) {
val thisParser = all(idx)
Expand All @@ -2180,7 +2195,7 @@ object Parser {
// we failed to parse, but didn't consume input
// is unchanged we continue
// else we stop
errs = errs ++ err
errs = errs.map(_ ++ err.value)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.value isn't safe. We should do for { e1 <- errs; e2 <- err } yield (e1 ++ e2) or (errs, err).mapN(_ ++ _) but maybe the latter is slightly slower due to dispatch via typeclass.

state.error = null
idx = idx + 1
}
Expand Down Expand Up @@ -2221,7 +2236,7 @@ object Parser {
}
}
if (lastMatch < 0) {
state.error = Chain.one(Expectation.OneOfStr(startOffset, all.toList))
state.error = Eval.later(Chain.one(Expectation.OneOfStr(startOffset, all.toList)))
state.offset = startOffset
} else {
state.offset = lastMatch
Expand Down Expand Up @@ -2645,7 +2660,8 @@ object Parser {
state.offset += 1
char
} else {
state.error = Chain.one(Expectation.InRange(offset, Char.MinValue, Char.MaxValue))
state.error =
Eval.later(Chain.one(Expectation.InRange(offset, Char.MinValue, Char.MaxValue)))
'\u0000'
}
}
Expand All @@ -2656,15 +2672,17 @@ object Parser {

override def toString = s"CharIn($min, bitSet = ..., $ranges)"

def makeError(offset: Int): Chain[Expectation] = {
var result = Chain.empty[Expectation]
var aux = ranges.toList
while (aux.nonEmpty) {
val (s, e) = aux.head
result = result :+ Expectation.InRange(offset, s, e)
aux = aux.tail
def makeError(offset: Int): Eval[Chain[Expectation]] = {
Eval.later {
var result = Chain.empty[Expectation]
var aux = ranges.toList
while (aux.nonEmpty) {
val (s, e) = aux.head
result = result :+ Expectation.InRange(offset, s, e)
aux = aux.tail
}
result
}
result
}

override def parseMut(state: State): Char = {
Expand Down Expand Up @@ -2703,7 +2721,7 @@ object Parser {
val matchedStr = state.str.substring(offset, state.offset)
// we don't reset the offset, so if the underlying parser
// advanced it will fail in a OneOf
state.error = Chain.one(Expectation.ExpectedFailureAt(offset, matchedStr))
state.error = Eval.later(Chain.one(Expectation.ExpectedFailureAt(offset, matchedStr)))
}

state.offset = offset
Expand Down Expand Up @@ -2732,7 +2750,7 @@ object Parser {
override def parseMut(state: State): A = {
val a = under.parseMut(state)
if (state.error ne null) {
state.error = state.error.map(Expectation.WithContext(context, _))
state.error = state.error.map(_.map(Expectation.WithContext(context, _)))
}
a
}
Expand All @@ -2742,7 +2760,7 @@ object Parser {
override def parseMut(state: State): A = {
val a = under.parseMut(state)
if (state.error ne null) {
state.error = state.error.map(Expectation.WithContext(context, _))
state.error = state.error.map(_.map(Expectation.WithContext(context, _)))
}
a
}
Expand Down