These are chat archives for mdedetrich/scala-json-ast

24th
Jul 2017
Ichoran
@Ichoran
Jul 24 2017 03:10
I think that's entirely reasonable: get a working representation for 1.0 and get a 1.1 out the door soon that has better performance.
Matthew de Detrich
@mdedetrich
Jul 24 2017 09:00
@eed3si9n The issue is more with VectorMap, I don’t know what the performance of it will be beforehand and this may change the API
i.e. if the VectorMap ends up being only slightly slower/take up slightly more memory then our current Map then it will be default constructor for JObject and unsafe will be removed entirely
Otherwise I will have to decide what to do

@Ichoran Regarding your comment earlier

That's some pretty wide fanout. In principle if encoding and decoding in memory is a likely scenario, this is the way to do it, but I'm skeptical that this is an important use case.

Can you explain this differently?

Matthew de Detrich
@mdedetrich
Jul 24 2017 09:16
@Ichoran @eed3si9n If you guys want to speed this up, it would be great if you could supply any information about an immutable map that maintains insertion/deletion order which has sane complexity
eugene yokota
@eed3si9n
Jul 24 2017 09:33
@mdedetrich like JsonNumber, Circe defines an abstract class JsonObject and hides the implementation. Parsed ones would use LinkedHashMap-based implementation, and when you modify them it switches to MapAndVectorJsonObject - https://github.com/circe/circe/blob/master/modules/core/shared/src/main/scala/io/circe/JsonObject.scala
Matthew de Detrich
@mdedetrich
Jul 24 2017 10:44

Indeed I know what Circe does, which is precisely the point, its hiding the internal value of JSON object (and this is also according to @travisbrown and Circe’s design goals where users shouldn’t deal with the AST at all).

This is different to scalajson designs goals, at least with Circe if you want to work with the JSON object you have to call .toMap on the object, which is different to ScalaJSON where the map of the object itself is directly exposed as a value

This all has implications on the API, and it rests on how performant an implementation of VectorMap is
Like if we are going down the road of hiding the internal representation, then this will completely change the design goals
eugene yokota
@eed3si9n
Jul 24 2017 16:34

ScalaJSON where the map of the object itself is directly exposed as a value

can't we fake this by defining apply and unapply?

Ichoran
@Ichoran
Jul 24 2017 17:31
Given the immense troubles people had even making Vector "fast enough" (meaning "not actually fast enough for any single-element stuff that you do seriously"), I'm skeptical that an eagerly-constructed VectorMap could be fast enough to satisfy everyone.
Also, my previous comment about all the different Number classes was really this: I don't think storing a Double in a JSON number, in memory, and then pulling it right back out again is a major use-case, so it doesn't really matter much what you do.
The inheritance version is cleaner, and shouldn't interfere too badly, even if you can optimize it a little better by hand with flags if you have good guesses about which instances will actually be used.
eugene yokota
@eed3si9n
Jul 24 2017 17:37
right. capturing the input parameter to JNumber.apply(..) via flag to some degree was a feature creep in other words. and my inheritance was a reaction/refactoring to that.
if we say that Double -> AST -> Double perf is not super useful, then neither flag nor JDouble is all that useful
Ichoran
@Ichoran
Jul 24 2017 17:38
I would also be perfectly comfortable coming right out and saying, "If you do that, it's slow. It'll parse the number again."
If we adopt sane rules for a Double and a JSON number being "the same", it won't matter. It only is really painful if you put a Double in something and then can't test that it's there due to rounding issues.
eugene yokota
@eed3si9n
Jul 24 2017 17:40
so another caveat should be "if you use Float or Double, don't expect to round trip"
Ichoran
@Ichoran
Jul 24 2017 17:49
No, no, you should expect to. We do it with strings; JSON is just a string; everything is cool there.
You just shouldn't expect to have a more discriminating way of telling that a Double is or is not encoded in a JSON value than seeing if what comes out of toDouble is finite.
It's the hyperaccuracy that makes the Double tests fail, not the Double-to-String-to-Double roundtrip.
Travis Brown
@travisbrown
Jul 24 2017 17:57
fwiw I'm getting rid of the JsonDouble constructor in circe 0.9.0 (because it complicates things in some upcoming refactoring and as you say double-to-ast-to-double isn't a use case I care about).