Input/Output refactor, breaking changes #74

ghik · 2018-07-09T18:44:49Z

This PR contains some controversial, breaking but IMHO necessary changes in serialization framework:

InputType has been dropped.

It was used only to check if Input contains null so it was replaced by simple isNull method. This change is primarily motivated by the fact that some "simple" types may need to use array/object encoding, e.g. #64 - this way array/object values would not be properly distinguishable from "simple" values and inputType method would have to lie in some way.

GenCodec for Option has been changed to use unwrapped encoding with null representing None.

My original motivation for using list-based encoding was to be able to express null and Some(null) values for Option. It was additionally supported by an assumption that we don't care too much about serialized format as long as deserializes properly. This is no longer true as GenCodec is now being used for various purposes and may affect the way our OSS is used outside the company (e.g. through Udash).

Currently, the only way to ensure unwrapped encoding of optional values is to use Opt/OptArg which I definitely don't want to impose on people using our libraries. For this reason I see no other choice but to introduce this breaking change and somehow manage the breakage in our own use cases which primarily involve serialization to MongoDB. Therefore, I left an ugly backdoor that Input and Output implementations may use to retain the previous encoding of Option for compatibility purposes - see LegacyOptionEncodingInput/LegacyOptionEncodingOutput.

Introduced JsonOptions for better control of output format of JsonStringOutput (timestamp format, binary data format, etc.). See documentation of JsonOptions for more details.

The main breaking changes in JSON serialization is that timestamps are now by default serialized as ISO formatted strings, e.g. "2010-04-10T08:50:41.262Z" and byte arrays are serialized as actual JSON (signed) byte arrays instead of hex strings. This of course can be adjusted in JsonOptions to use previous format.

Introduced general purpose Timestamp type that serves as a minimal, platform-independent representation of UTC based instant.

ddworak · 2018-07-13T05:51:26Z

commons-core/js/src/main/scala/com/avsystem/commons/jsiop/JsInterop.scala

+
+import scala.scalajs.js
+
+trait JsInterop {


A JsInterop companion would be nice, not everyone is so keen to inherit helpers or import the whole world from commons.

ddworak · 2018-07-13T05:57:39Z

commons-core/jvm/src/main/scala/com/avsystem/commons/jiop/JavaTimeInterop.scala

+import com.avsystem.commons.misc.TimestampConversions
+
+trait JavaTimeInterop {
+  implicit def instantTimestampConversions(instant: Instant): TimestampConversions =


Add JDate to the mix, it's not my favourite either, but it's still widely used. It's also the only JVM time type present in SJS as well.

It's already in JBasicUtils because JDate is available also in ScalaJS while JavaTimeInterop is JVM only.

ddworak · 2018-07-13T06:12:25Z

commons-core/src/main/scala/com/avsystem/commons/misc/Timestamp.scala

+  *
+  * @param millis milliseconds since UNIX epoch, UTC
+  */
+class Timestamp(val millis: Long) extends AnyVal {


Any reason for it not to be final?

Value classes are always final

ddworak · 2018-07-13T06:18:46Z

commons-core/src/main/scala/com/avsystem/commons/misc/Timestamp.scala

+  *
+  * @param millis milliseconds since UNIX epoch, UTC
+  */
+class Timestamp(val millis: Long) extends AnyVal {


I really like that we'll finally have a general-purpose cross-compiled timestamp representation in commons, but it will be often useless for SJS frontends without DateTime-like formatting. Our go-to solution for now is to transform JVM formats to Moment.js formats, which binds us to JDate most of the time anyway.

I wanted to have a sensible cross-compiled, immutable, comparable, serializable and human-readable type that could be put into RPC interfaces.

I know that in order to show it to the user, fronted will have to convert it to something raw (millis, js.Date) so it can be passed to moment.js or whatever other formatting library. However, this can now happen at the edge of our codebase and JDate will no longer pollute it, most importantly our RPC interfaces.

ddworak · 2018-07-13T06:23:48Z

commons-core/src/main/scala/com/avsystem/commons/misc/Timestamp.scala

+import scala.concurrent.duration.{FiniteDuration, TimeUnit}
+
+/**
+  * Millisecond-precision, general purpose, cross compiled timestamp representation.


Instant has nanosecond resolution. JS has performance.now() with kinda-usually-microseconds resolution (browser dependent). I wouldn't introduce a new, less precise representation as an intermediate one.

Yes, I am aware of the precision loss but I think it's still worth the cost of having to remember about it.

I'm not sure what you mean by this intermediate precision.

I mean that this new representation has neat conversions to all the different "times" one can have to use. Its loss of precision disables us from using it as an intermediate format.

Precision loss happens only at converting Instant to Timestamp - all the other conversions are fine. I can change the name of toTimestamp extension method on Instant to truncateToTimestamp so that it's clear that it loses precision. WDYT?

I don't think we need cross-compiled instant data type with more than millisecond precision.

ddworak · 2018-07-13T06:54:53Z

commons-core/src/main/scala/com/avsystem/commons/serialization/json/JsonOptions.scala

+sealed trait JsonDateFormat
+object JsonDateFormat {
+  /**
+    * Specifies that a timestamp should be represented in ISO 8861 format with UTC time zone,


ISO 8861? 🤣 https://www.evs.ee/products/iso-8861-1998

ddworak · 2018-07-13T06:58:48Z

commons-core/src/main/scala/com/avsystem/commons/serialization/json/JsonStringInput.scala

+    case JsonBinaryFormat.ByteArray =>
+      val builder = new mutable.ArrayBuilder.ofByte
+      val li = readList()
+      while (li.hasNext) {


The Inputs/Outputs could look nicer if e.g. ListInputs were Traversable(Once)

Absolutely not, I will not make it inherit collections API. Inputs and Outputs are as raw as possible.

ddworak · 2018-07-13T07:07:21Z

commons-core/src/main/scala/com/avsystem/commons/serialization/json/JsonStringInput.scala

+    case JsonBinaryFormat.HexString =>
+      val hex = checkedValue[String](JsonType.string)
+      val result = new Array[Byte](hex.length / 2)
+      var i = 0


(0 until result.length).map(... should not be much slower and you could even go all the way .toArray

I want this as raw and fast as possible.

Benchmark it then 😃

I will. But on the first sight your version should be much slower because not only will it create intermediate collection but also every byte will be boxed in that collection.

Introducing foreach was relatively harmless but map + toArray cut off more than half of benchmark score. Ultimately I made a tail-recursive loop.

ddworak · 2018-07-13T07:08:46Z

commons-core/src/main/scala/com/avsystem/commons/serialization/json/JsonStringOutput.scala

    sb.toString
  }
 }

 trait BaseJsonOutput {
-  protected final def writeJsonString(builder: JStringBuilder, str: String): Unit = {
+  protected final def indent(builder: JStringBuilder, options: JsonOptions, depth: Int): Unit =
+    options.indentSize match {


Again, I don't want that lambda in here - this is called often and I want this as fast as possible.

ddworak · 2018-07-13T07:10:11Z

commons-core/src/main/scala/com/avsystem/commons/serialization/legacyOptionEncoding.scala

+package serialization
+
+/**
+  * This ugly workaround has been introduced when standard `Option` encoding changed from zero-or-one element list


Don't be so hard on yourself, seen worse than that.

Starzu

I'm worried about the performance of GenCodec[Option[T]] with these LegacyOptionEncoding input and output. I think we should check it with some benchmarks.

Starzu · 2018-07-13T06:22:54Z

commons-core/js/src/main/scala/com/avsystem/commons/misc/TimestampConversions.scala

+
+final class TimestampConversions(private val millis: Long) extends AnyVal {
+  def toJsDate: js.Date = new js.Date(millis.toDouble)
+  def toJDate: JDate = new JDate(millis)


toTimestamp?

Starzu · 2018-07-13T06:29:51Z

commons-core/js/src/main/scala/com/avsystem/commons/serialization/IsoInstant.scala

+
+  def parse(string: String): Long = {
+    def fail = throw new ReadFailure(s"invalid ISO instant: $string")
+    if (regex.test(string)) {


Probably this regex test makes the whole method much slower. This affects JsonStringInput, so could you at least measure the difference?

Very naive benchmark shows no significant difference (5-20%).
I want this to be secured with this regex because I want this to work as closely as possible in both JS and JVM.

Starzu · 2018-07-13T06:30:21Z

commons-core/jvm/src/main/scala/com/avsystem/commons/misc/TimestampConversions.scala

+
+final class TimestampConversions(private val millis: Long) extends AnyVal {
+  def toInstant: Instant = Instant.ofEpochMilli(millis)
+  def toJDate: JDate = new JDate(millis)


toTimestamp

Starzu · 2018-07-13T06:45:47Z

commons-core/src/main/scala/com/avsystem/commons/serialization/GenCodec.scala

+
+  implicit def optionCodec[T: GenCodec]: GenCodec[Option[T]] = create[Option[T]](
+    locally {
+      case i: LegacyOptionEncodingInput if i.legacyEncodingEnabled =>


This pattern matching looks like something slow. Could you create a benchmark to compare this version with clean codec?

That's just an instanceof check - I don't see a reason why would be particularly slow.
But yeah, measuring is always good.

https://youtu.be/e4TKlLAlYaA?t=30m22s

👍 will check that

Yes, you were right - now I just left a method in Input & Output instead of having it in separate traits.

Starzu · 2018-07-13T07:02:19Z

commons-core/src/main/scala/com/avsystem/commons/serialization/json/JsonOptions.scala

+/**
+  * Specifies format used by `JsonStringOutput.writeBinary` / `JsonStringInput.readBinary` to represent byte arrays.
+  */
+sealed trait JsonBinaryFormat


I see lots of JS generated from this file. :)

Could be a ValueEnum but I wanted to leave possibility to make a case class there.

Starzu · 2018-07-16T05:55:09Z

commons-core/src/main/scala/com/avsystem/commons/serialization/json/JsonStringInput.scala

-      while (i < result.length) {
-        result(i) = ((reader.fromHex(hex.charAt(2 * i)) << 4) | reader.fromHex(hex.charAt(2 * i + 1))).toByte
-        i += 1
+      def loop(i: Int): Unit = if (i < result.length) {


Starzu · 2018-07-16T06:03:26Z

commons-benchmark/js/src/main/scala/com/avsystem/commons/Main.scala

 import japgolly.scalajs.benchmark.gui.BenchmarkGUI
 import org.scalajs.dom._

 object Main {
  def main(args: Array[String]): Unit = {
    val body = document.getElementById("body")
-    BenchmarkGUI.renderSuite(body)(Benchmarks.suite)
+    BenchmarkGUI.renderSuite(body)(IsoInstantBenchmarks.suite)


BenchmarkGUI.renderMenu(body)( IsoInstantBenchmarks.suite, JsonBenchmarks.suite )

ghik added 2 commits July 9, 2018 17:03

replaced InputType with simple isNull

6f2170a

dropped list encoding of Option with ugly legacy compatibility backdoor

57d4b48

ghik requested review from Starzu, ddworak and mkej July 9, 2018 18:44

ghik added 15 commits July 9, 2018 20:45

trailing comma

c4f6c5a

configurable binary & timestamp format for JsonStringInput/Output

cd4460e

asciiOutput option for JSON

5a629b1

added configurable MathContext for parsing BigDecimals from JSON

33ef23a

introduced option to limit JSON numbers to JS-parseable ones

1d55fcb

added simple indent option to JsonStringOutput

e168319

removed bigNumbers from JsonOptions because JS is worthless anyway

ae82546

removed unnecessary toString calls

e061c75

manually excluded Seq.apply/unapplySeq from applyUnapplyFor

0b948d3

JsonOptions doc

0d52367

improved IsoInstant format/parse

fd7e8dc

more test fixes

dabd186

align implementations of IsoInstant between platforms

5091d5b

general purpose Timestamp type with various interop utils

3b6b5c0

cosmetic

e7ed0b2

ddworak reviewed Jul 13, 2018

View reviewed changes

Starzu suggested changes Jul 13, 2018

View reviewed changes

ghik added 6 commits July 13, 2018 14:19

review improvements

30ea0fc

removed LegacyOptionEncodingInput/Output traits for performance

f79d8af

benchmarks for hex string binary JSON format

adf0b90

IsoInstantBenchmarks for JS

4bdf37a

scaladoc fix

5ce474d

Merge branch 'master' into io-refactor

81fdf1b

Starzu reviewed Jul 16, 2018

View reviewed changes

Starzu approved these changes Jul 16, 2018

View reviewed changes

ddworak approved these changes Jul 16, 2018

View reviewed changes

more review changes

19ab22b

ghik merged commit b828541 into master Jul 16, 2018

ghik deleted the io-refactor branch July 16, 2018 12:59

This was referenced Jul 16, 2018

Configurable JsonStringOutput/Input #66

Closed

Parsed BigDecimal is not limited for a scale value and has an UNLIMITED math context that is unsafe for open systems #65

Closed

GenCodec serializes byte arrays to a base16 encoded string #64

Closed

ghik restored the io-refactor branch January 11, 2019 12:49

ghik deleted the io-refactor branch January 11, 2019 13:04

Input/Output refactor, breaking changes #74

Input/Output refactor, breaking changes #74

Uh oh!

Conversation

ghik commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghik Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghik Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghik Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Starzu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghik commented Jul 9, 2018 •

edited

Loading

ghik Jul 13, 2018 •

edited

Loading

ghik Jul 13, 2018 •

edited

Loading

ghik Jul 13, 2018 •

edited

Loading