Improve RichTextCodec decoding performance
#2512
Conversation
987Nabil
requested review from
amitksingh1490,
jdegoes,
vigoo and
adamgfraser
as code owners
Error messages are the same per instance of `CharIn`. So we compute them only once.
987Nabil
force-pushed
the
rich-text-codec-char-in-opt
branch
from
3502bad
to
52f2f49
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2512 +/- ##
==========================================
- Coverage 64.94% 64.90% -0.04%
==========================================
Files 137 137
Lines 7243 7247 +4
Branches 1248 1264 +16
==========================================
Hits 4704 4704
- Misses 2539 2543 +4 ☔ View full report in Codecov by Sentry. |
|
@987Nabil just leaving this comment here for visibility. Thanks for the fix but just want to raise a concern with this approach. It "fixes" the latency the issue in situations that an instance of the I'm sure you already thought of different approaches, but does the error message needs to contain every single character in the BitSet? Perhaps it can be limited to the first 5 or 10 or even 50? If I'm understanding the code correctly, it currently contains all 65535 chars, and I'm not too sure whether that's really useful. |
| @@ -154,15 +154,17 @@ sealed trait RichTextCodec[A] { self => | |||
| } | |||
| object RichTextCodec { | |||
| private[codec] case object Empty extends RichTextCodec[Unit] | |||
| private[codec] final case class CharIn(set: BitSet) extends RichTextCodec[Char] | |||
| private[codec] final case class CharIn(set: BitSet) extends RichTextCodec[Char] { | |||
| lazy val errorMessage = s"Not found: ${set.toArray.map(_.toChar).mkString}" | |||
There was a problem hiding this comment.
You can do one better by removing lazy, and adding:
private[codec] val leftErrorMessage = Left(errorMessage)Then using leftErrorMessage below.
|
@kyri-petrou I see your point. I must say, the need for going with |
|
We discussed using |
| @@ -250,6 +250,10 @@ object RichTextCodecSpec extends ZIOHttpSpec { | |||
| assertTrue(success(123) == codec.decode("123--")) && | |||
| assertTrue(codec.decode("4123").isLeft) | |||
| }, | |||
| test("With error message") { | |||
| val codec = RichTextCodec.literal("123").withError("Not 123") | |||
| assertTrue(codec.decode("678") == Left("(Expected, but did not find: Paragraph(Code(“1”,Inline)), Not 123)")) | |||
There was a problem hiding this comment.
This is rendering poorly, perhaps because describe returns Doc, that should first be properly converted into a string.
There was a problem hiding this comment.
The problem is actually the | operator, that chains error messages this way explicitly.
987Nabil
force-pushed
the
rich-text-codec-char-in-opt
branch
2 times, most recently
from
bcfb4f6
to
8a3d75f
Compare
987Nabil
force-pushed
the
rich-text-codec-char-in-opt
branch
from
8a3d75f
to
cc451fc
Compare
Error messages are the same per instance of
CharIn. So we compute themonly once.