[jsonschema] Rethink how jsonschema deals with encoding internally about rest-layer

[yanfali]

@smyrman I agree with the benchmarks though. I would like to know how much slower it is than a hand coded version.

[yanfali]

I think this is worth attempting. I just took a look at https://golang.org/src/encoding/json/encode.go and it's pretty much switch statements and writing to buffers, but if you can make the code less error prone that's a worth goal. Writing serializers and deserializers tends to be a low level and messy business.

[smyrman]

and it's pretty much switch statements and writing to buffers

Yeah, but they also have a separate type for each encoder (intEncoder, uintEncoder etc.). See https://golang.org/src/encoding/json/encode.go#L391.

Writing serializers and deserializers tends to be a low level and messy business.

I have actually written a binary one, and yes I agree. The biggest question is which solution to go with... I have three (new) proposals that I have added to #35, as I am thinking about using the same interface externally (public) as we use internally. Each solution have some advantages and disadvantages.

[smyrman]

FYI, I now have a working plan on this, which involves 3 steps.

First of all, I will create some micro-benchmarks for the jsonschema package, and add this as a separate PR.

Secondly, I will clean up the code by splitting the encoder into separate files with one encoder per supported type, mirroring the layout of the schema package. The encoder logic itself will be more or less exactly the same as it is now, except the encoder-logic for each type will move into a separate function. This will be the second PR. Most likely, I will pass in an io.Writer rather than an errWriter to each encoder.

Thirdly, I really want to try out solution 2 from #35, which involves getting rid of all the JSON encoder logic altogether, and simply deal with map[string]interface{} internally, and only use the standard-library JSON encoder. This would be in line with how API payloads are serialized/deserialized. From the discussion in #35, this proposal does not necessarily have any buy-in yet, so this PR might be rejected. However, if writing serializers and deserializers are messy and low level, surely avoiding it altogether would make for a more robust, although worse performing, solution. Dealing with maps, also holds the door open for end-users to implement other encodings (e.g. YAML) to be used for JSON Schema / Swagger, similar to how the rest package today allows for a custom ResponseSender.

[yanfali]

One thought I have about the performance is that it would be fairly easy to memoize the output after a single run so we trade memory for speed and amortize the cost. Right now these things run fairly infrequently for me and are mostly informational so the performance isn't a huge concern.

[smyrman]

Yes, I agree to that @yanfali. The performance of JSON Schema generation probably hardly matters to anyone.

[smyrman]

I guess that really means that the micro benchmarks (step 1), aren't really going to be necessary. I will write some anyway, as I am quite interested in seeing the difference in performance / number of allocations between the two approaches on a general basis, even though encoding performance isn't critical here.

UPDATE: I can submit a PR for them, but I am happy if we decide to reject them.

[smyrman]

OK, the code for a map[string]interface{} based encoding in JSONSchema is ready, and so are the benchmark results. I will wait with the PR unitl #70 is merged, but you may pre-inspect the code here.

I think the code is really much cleaner, but a bit slower, especially for tiny schemas. Compared to #70, here are the benchmark results from my machine:

benchmark                                                    old ns/op     new ns/op     delta
BenchmarkEncoder/Schema={Fields:{"b":Bool{}}}                1318          5405          +310.09%
BenchmarkEncoder/Schema={Fields:{"s":String{}}}              1249          5270          +321.94%
BenchmarkEncoder/Schema={Fields:{"s":String{MaxLen:42}}}     1550          6063          +291.16%
BenchmarkEncoder/Schema=Student                              9797          20723         +111.52%

benchmark                                                    old allocs     new allocs     delta
BenchmarkEncoder/Schema={Fields:{"b":Bool{}}}                14             40             +185.71%
BenchmarkEncoder/Schema={Fields:{"s":String{}}}              14             40             +185.71%
BenchmarkEncoder/Schema={Fields:{"s":String{MaxLen:42}}}     16             44             +175.00%
BenchmarkEncoder/Schema=Student                              59             136            +130.51%

benchmark                                                    old bytes     new bytes     delta
BenchmarkEncoder/Schema={Fields:{"b":Bool{}}}                496           2225          +348.59%
BenchmarkEncoder/Schema={Fields:{"s":String{}}}              480           2225          +363.54%
BenchmarkEncoder/Schema={Fields:{"s":String{MaxLen:42}}}     504           2344          +365.08%
BenchmarkEncoder/Schema=Student                              2528          7033          +178.20%

Machine details:

    Hardware Overview:

      Model Name: MacBook Pro
      Model Identifier: MacBookPro11,1
      Processor Name: Intel Core i5
      Processor Speed: 2.6 GHz
      Number of Processors: 1
      Total Number of Cores: 2
      L2 Cache (per Core): 256 KB
      L3 Cache: 3 MB
      Memory: 8 GB

Commands to reproduce:

$ git checkout ... # old code
$ go test -run=NONE -bench=. -benchmem -cpu 1 > 1.txt
$ git checkout ... # new code
$ go test -run=NONE -bench=. -benchmem -cpu 1 > 2.txt
$ benchcmp 1.txt 2.txt

[yanfali]

Not near a good internet connection, but it would be good to see what the impact is on a more substantial schema. small schemas encode and decode so fast it's probably not a big deal. How about an array containing an object containing an array containing a different object. Each object should have at least 6 fields. Realistic ones are probably ints, strings and time objects. When I get back to civilization I can come up with a better example. Thanks

…

On Fri, Dec 23, 2016 at 1:35 PM Sindre Røkenes Myren < ***@***.***> wrote: OK, the code for a map[string]interface{} based encoding in JSONSchema is ready, and so are the benchmark results. I will wait with the PR unitl #70 <#70> is merged, but you may pre-inspect the code here <https://github.com/smyrman/rest-layer/tree/jsonschema-map-encode>. I think the code is really much cleaner, but a bit slower, especially for tiny schemas. Compared to #70 <#70>, here are the benchmark results from my machine: benchmark old ns/op new ns/op delta BenchmarkEncoder/Schema={Fields:{"b":Bool{}}} 1318 5405 +310.09% BenchmarkEncoder/Schema={Fields:{"s":String{}}} 1249 5270 +321.94% BenchmarkEncoder/Schema={Fields:{"s":String{MaxLen:42}}} 1550 6063 +291.16% BenchmarkEncoder/Schema=Student 9797 20723 +111.52% benchmark old allocs new allocs delta BenchmarkEncoder/Schema={Fields:{"b":Bool{}}} 14 40 +185.71% BenchmarkEncoder/Schema={Fields:{"s":String{}}} 14 40 +185.71% BenchmarkEncoder/Schema={Fields:{"s":String{MaxLen:42}}} 16 44 +175.00% BenchmarkEncoder/Schema=Student 59 136 +130.51% benchmark old bytes new bytes delta BenchmarkEncoder/Schema={Fields:{"b":Bool{}}} 496 2225 +348.59% BenchmarkEncoder/Schema={Fields:{"s":String{}}} 480 2225 +363.54% BenchmarkEncoder/Schema={Fields:{"s":String{MaxLen:42}}} 504 2344 +365.08% BenchmarkEncoder/Schema=Student 2528 7033 +178.20% Machine details: Hardware Overview: Model Name: MacBook Pro Model Identifier: MacBookPro11,1 Processor Name: Intel Core i5 Processor Speed: 2.6 GHz Number of Processors: 1 Total Number of Cores: 2 L2 Cache (per Core): 256 KB L3 Cache: 3 MB Memory: 8 GB Commands to reproduce: ``bash $ git checkout ... # old code $ go test -run=NONE -bench=. -benchmem -cpu 1 > 1.txt $ git checkout ... # new code $ go test -run=NONE -bench=. -benchmem -cpu 1 > 2.txt $ benchcmp 1.txt 2.txt — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#51 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACK5U4mMSaSEmOs7f5Qzuep7s1kiHcGlks5rLD6FgaJpZM4LBSe2> .

[smyrman]

The most representable one atm. is probably the "Schema=Student" test, which is a small schema, but not a tiny one. It is a bit more than twice as slow with ~20ms v.s. ~10ms before. The tiny examples are more than 4x slower. If I were to guess, I would say there is probably a 3ms added overhead with this solution and a ~2x slowdown.

I think for the case of API documentation, this is acceptable. But if it was for the main payload transferred by the API, it probably would not be.

How about an array containing an object containing an array containing a different object. Each object should have at least 6 fields.

Yeah, you can do so if you like. I don't know how much the nesting matters, as also fields get their own maps, so I am guessing it might perform similar to a flat schema with 10+ fields. Probably a bit slower since the map allocated for a Schema is larger than the one allocated for fields.

[rs]

Can we close this issue now?

[smyrman]

Yes