Defer creation of Expectations #280
Conversation
Codecov Report
@@ Coverage Diff @@
## main #280 +/- ##
==========================================
+ Coverage 96.32% 96.35% +0.02%
==========================================
Files 8 8
Lines 980 988 +8
Branches 93 92 -1
==========================================
+ Hits 944 952 +8
Misses 36 36
Continue to review full report at Codecov.
|
|
The win looks really good and I'm definitely interested in merging when green and we sort out any mima issues. |
|
Drat. I was afraid stack safety would be an issue with the thunks. I'll pause here and see if people agree with the problem before I pour more into the solution. |
|
I don't see why Eval should hurt much and I imagine flatMap can solve the stack issues no? |
This reverts commit dedd158.
|
A thunk is slightly lighter than Replacing |
|
Do you want a benchmark added to this project? A dumbed down HTTP header parser should do it. If I can get this reasonably competitive with the gross handwritten ones in http4s, a proper one will appear there. |
| Chain.fromSeq( | ||
| ranges.toList.map { case (s, e) => | ||
| Expectation.InRange(offset, s, e) | ||
| } | ||
| ) |
There was a problem hiding this comment.
@rossabaker last time I saw this #62 are you seeing diff results? maybe worth keep?
There was a problem hiding this comment.
It won't show up in my benchmark since we now avoid this path instead of calling it a zillion times. It probably doesn't matter in reality because it's now only called once per failed parse. But I guess there's no reason not to keep the optimization and fail as fast as we can. I'll restore it.
Now that I'm aware of #62, I suspect we see some positive effect from this on those benchmarks.
|
I need my CPU back so I can't run a proper number of iterations right now, but looking good on the JSON:
Before, on 9e67814: After, on 9561249: Error on both: |
|
The json parser got refactored in a way that broke it I think and we aren't testing the code in CI so we didn't noticed. This was fixed for the read me here: |
| 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) |
There was a problem hiding this comment.
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?
| @@ -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) | |||
There was a problem hiding this comment.
.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.
|
I'll take the changes I requested. Thanks for sending a PR! |
Given a parser whose hot part looks like this:
CharIn.makeErroris dominating the stack trace and allocations. It is created at the end of each repetition, only to be nulled out and gc'ed if we've made any progress.Here, we wrap every
Chain[Expectation]in anEval, so that the expectations can be created lazily. Care must be taken not to close over mutable state, namely,state.offset. We significantly increase speed and reduce garbage:I'll work on cleaning up and providing a runnable benchmark, but wanted to get early feedback.
We might also make it leaner with simple thunks rather than an
Evalthat's alwayslater.