private/protocol/json/jsonutil: Use json.Decoder to decrease memory allocation

[atsushi-ishibashi]

closed: #2114

https://gist.github.com/atsushi-ishibashi/2d7d13cf1df9a84251ed175816a04b14
The below shows that the new UnmarshalJSON decrease memory capacity by 50%.

//current
BenchmarkUnmarshalJSON-4    	 1000000	      2122 ns/op	    2928 B/op	      18 allocs/op
//new one
BenchmarkUnmarshalJSON2-4   	 1000000	      1654 ns/op	    1480 B/op	      17 allocs/op

[jasdel]

Thanks for putting this change together @atsushi-ishibashi Thanks for finding this optimization. We'll run this through our integration tests, but I don't immediately see any reason why we shouldn't be able to accept this change.

[jasdel]

Noticed an issue with checking for io.EOF from the Decode. I don't think these lines are needed.

[jasdel]

Did you find a case where the io.EOF was being returned, and it wasn't a parsing error? I wouldn't of expected Decode to return an EOF error if it successfully read the content.

I think the if err == io.EOF { block can be removed.

[atsushi-ishibashi]

This may be compatible with the below in the previous code.

if len(b) == 0 {
	return nil
}

If there isn't if err == io.EOF {, the new UnmarshalJSON returns error with 0 bytes stream.

But I cloudn't confirm where the scope is the same:thinking:

[xibz]

@atsushi-ishibashi - Oof, I really don't like relying on the io.EOF as it requires internal knowledge on how Decode works. However, I think we can accept this change with one minor tweak

err := json.NewDecoder(stream).Decode(&out)
if err == io.EOF {
    return nil
} else if err != nil {
    return err
}

It may be a good idea of not creating a new decoder every time either. We can let UnmarshalJSON handle the creation of the stream and pass that into unmarshalJSON. Let me know what you think of that!

// or maybe make this a method on an object? That way decoder doesn't need to be passed
// down
func unmarshalJSON(decoder json.Decoder, v interface{}, stream io.Reader) error {
    err := decoder.Decode(stream)
    // same logic as above
}

[atsushi-ishibashi]

I really don't like relying on the io.EOF as it requires internal knowledge on how Decode works.

Completely agree👍I'll modify the way of error handling following with your suggestion.

It may be a good idea of not creating a new decoder every time either.

I don't agree with passing decoder as an argument because it will change interface. Currently UnmarshalJSON is package's public fuction so one possible choice is that creating decoder as package variable in init fuction or variable initialization. But we have to consider that there is a way to reassign io.Reader to Docoder. As far as I examined in a few minutes, it doesn't seem to exist:thinking:
When I misunderstand your suggestion, please correct me!

[xibz]

nvm! Looks like UnmarshalJSON will ever only be called once! Forget I said anything :D

[xibz]

@atsushi-ishibashi - Oof, I really don't like relying on the io.EOF as it requires internal knowledge on how Decode works. However, I think we can accept this change with one minor tweak

err := json.NewDecoder(stream).Decode(&out)
if err == io.EOF {
    return nil
} else if err != nil {
    return err
}

It may be a good idea of not creating a new decoder every time either. We can let UnmarshalJSON handle the creation of the stream and pass that into unmarshalJSON. Let me know what you think of that!

// or maybe make this a method on an object? That way decoder doesn't need to be passed
// down
func unmarshalJSON(decoder json.Decoder, v interface{}, stream io.Reader) error {
    err := decoder.Decode(stream)
    // same logic as above
}