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

Experiment - eliminate creation of nested tuples #78

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sergei-shabanau
Copy link
Member

@sergei-shabanau sergei-shabanau commented Mar 25, 2020

Connects #75

Ultimately I am pursuing the following flow:

  1. Have user-defined AST1 in terms of Grammar.GADT
  2. Rewrite / Optimize AST1 into another AST2 expressed as low level GADT
  3. Compile resulting AST2 into parser/printer functions

My biggest concern is the lost type safety at step 2 with my current approach, thus I'd like to get some advise with implementation approach I am taken.

I've added inline comments to ease the discussion.

@sergei-shabanau sergei-shabanau force-pushed the 75-ast-zip-optimization branch from 53c0c13 to 1de1146 Compare March 25, 2020 19:39
@sergei-shabanau sergei-shabanau force-pushed the 75-ast-zip-optimization branch from 4f33a3c to fb43902 Compare March 25, 2020 21:19
@codecov-io
Copy link

codecov-io commented Mar 25, 2020

Codecov Report

Merging #78 into master will decrease coverage by 8.19%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##            master      #78      +/-   ##
===========================================
- Coverage   100.00%   91.80%   -8.20%     
===========================================
  Files            3        3              
  Lines          269      293      +24     
  Branches         5        6       +1     
===========================================
  Hits           269      269              
- Misses           0       24      +24     
Impacted Files Coverage Δ
...ain/scala/org/spartanz/parserz/ParsersModule.scala 90.87% <0.00%> (-9.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 459cdb9...7139c68. Read the comment docs.

Comment on lines 37 to 44
sealed trait Equiv[A, Repr]
case class Eq3[Z, A, B, C](f: (A, B, C) => Z, g: Z => Option[(A, B, C)]) extends Equiv[((A, B), C), Z]

object Equiv {
// def caseClass1[Z, A](f: A => Z, g: Z => Option[A]): Equiv[A, Z] = ???
// def caseClass2[Z, A, B](f: (A, B) => Z, g: Z => Option[(A, B)]): Equiv[(A, B), Z] = ???
def caseClass3[Z, A, B, C](f: (A, B, C) => Z, g: Z => Option[(A, B, C)]): Equiv[((A, B), C), Z] = Eq3(f, g)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Equiv is used to enforce type safety for conversion between nested Tuple types to Case Class types

Comment on lines +287 to +303
case Grammar.GADT.ZipUnsafe(gs) =>
(s: S, in: Input) => {
val size = gs.length
val arr: Array[Any] = Array.ofDim(size)
var i = 0
var state: S = s
var input: Input = in
var res: E \/ (Input, Array[Any]) = Right((input, arr))
while (i < size && res.isRight) {
val (s1, res1): (S, E \/ (Input, Any)) = parser(gs(i))(state, input)
res1.foreach { case (i1, v1) => input = i1; arr(i) = v1 }
state = s1
res = res1.map(_ => (input, arr))
i += 1
}
(state, res)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I have added ZipUnsafe into existing algebra for demo.
Correct implementation is to have Grammar.GADT compiled into another low level algebra, which in turn will be used to produce parser/printer functions.

Comment on lines +108 to +129
println(parser(g1)("", List('1', 'T')))
println()
println(parser(g2)("", List('1', 'T')))
println()
println(parser(g3)("", List('1', 'T')))
println()

println()
println(printer(g1)("", (Nil, ((2, true), "printed"))))
println()
println(printer(g2)("", (Nil, Thing(2, true, "printed"))))
println()
println(printer(g3)("", (Nil, Thing(2, true, "printed"))))
println()

println()
println(bnf("g1" @@ g1).mkString("\n"))
println()
println(bnf("g2" @@ g2).mkString("\n"))
println()
println(bnf("g3" @@ g3).mkString("\n"))
println()
Copy link
Member Author

Choose a reason for hiding this comment

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

Running yields same results for all cases

(,Right((List(),((1,true),blah))))
(,Right((List(),Thing(1,true,blah))))
(,Right((List(),Thing(1,true,blah))))

(,Right(List(2, T)))
(,Right(List(2, T)))
(,Right(List(2, T)))

<g1> ::= <one> <two> <thr>
<g2> ::= <one> <two> <thr>
<g3> ::= <one> <two> <thr>

Comment on lines 47 to 69
def toZ[A, AA, Z](g: G[A])(implicit equiv: Equiv[AA, Z], ev: AA <:< A): G[Z] = {
val g1: Option[GADT.ZipUnsafe[S, S, E]] = equiv match {
case Eq3(_, _) => zippy(3)(g).map(l => GADT.ZipUnsafe(l.toArray))
}
// g1.fold {
// toZDirect(g)(equiv, ev)
// ???
// } { g2 =>
equiv match {
case eq: Eq3[Z, ta, tb, tc] =>
GADT.Map[S, S, E, Array[Any], Z](
g1.get,
arr => {
val Array(a: ta, b: tb, c: tc) = arr
Right(eq.f(a, b, c))
},
z => {
eq.g(z).map { case (a, b, c) => Array(a, b, c) }.toRight("some error")
}
)
}
// }
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The only new thing available in public API as combinator (Equiv is private, GADT.ZipUnsafe is private).
Implementation benefits from the intended optimization: sequential ~ statements are rewritten into a new operator that produces an Array[_] of values instead of nested tuples. A Map operator is created in place of all the Zips.

Comment on lines 86 to 105
object Hacks {
def zippy[A](size: Int)(g: G[A]): Option[List[G[Any]]] = {

@scala.annotation.tailrec
def step[AA](i: Int)(acc: List[G[Any]])(g: G[AA]): List[G[Any]] =
g match {
// case GADT.ZipL(left, right, b) =>
// case GADT.ZipR(left, right, a) =>
case GADT.Zip(l, r) if i > 0 =>
step(i - 1)(r.asInstanceOf[G[Any]] :: acc)(l)
case g if i > 0 =>
g.asInstanceOf[G[Any]] :: acc
case _ =>
acc
}

val list = step(size)(Nil)(g)
if (list.length == size) Some(list) else None
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the place I am concerned about.
Using a list to keep Grammar[A] requires to loose types A.
Otherwise I have to do something like shapeless HList, or to go with Zip22 approach.
😕

@sergei-shabanau sergei-shabanau requested a review from jdegoes March 26, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants