-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add msgpack handler for smarter JSON encoding #7154
Conversation
@jsternberg, thanks for your PR! By analyzing the annotation information on this pull request, we identified @e-dard, @gunnaraasen and @joelegasse to be potential reviewers |
e90bb73
to
783654a
Compare
Sample decoder in Ruby: require "msgpack"
require "date"
require "excon"
r = Excon.post("http://localhost:8086/query",
body: URI.encode_www_form(db: "mydb", q: "SELECT * FROM cpu"),
headers: { "Accept" => "application/x-msgpack", "Content-Type" => "application/x-www-form-urlencoded" }
)
u = MessagePack::Unpacker.new(StringIO.new(r.body))
u.register_type(0x01) { |data|
nsecs = MessagePack.unpack(data)
Time.at(nsecs/1000000000, (nsecs%1000000000)/1000.0)
}
u.each do |v|
p v
end |
I'm curious how much of performance improvement we'd get from this as well. According to these benchmarks, might be fairly significant for larger result sets. |
I had tried it at some point when I originally implemented it and the speed results weren't that great (sometimes worse). But, we should have proper benchmarks for all of these along with proper unit tests for the content types that aren't JSON. I think the size was a decent amount less (like maybe a 10% decrease) but I'm trying to remember this from memory. |
783654a
to
f0e5125
Compare
Here's a few benchmarks of each response writer. Code is included in the PR so someone can check I got the code correct. I'm really bad at coding benchmarks.
|
f0e5125
to
6442c52
Compare
I found that the code to encode the time was very slow. I optimized it and now I get these times:
The only problem is the way that it is optimized does not allow for thread safety. We remake the codec for every response so this shouldn't be an issue, but I just wanted to say that in case there were some objections. I was able to get good times, but not as good, using a more threadsafe method. The thread safety should only be a problem if we use this to register a global codec. |
Something to add to this that is confusing the hell out of me. When I isolate the handlers in benchmarks, the msgpack one performs much better. When I try to use it within the program itself, it performs worse. I just ran a test where it took msgpack 35 seconds to encode a message and json took 25 seconds. |
6442c52
to
bcb3943
Compare
I think I finally figured it out. The This also explains why I couldn't find it in benchmarking. In benchmarking, I was always either discarding the output or it was being internally buffered with |
Found a few bugs while working on this in #7163. I'm going to wait for that to be merged and then merge it into master before this change should be considered for merging. |
52443cb
to
f6717ae
Compare
I switched this to use
When I tried it on a I did talk a bit with @goller though yesterday and I think this might be relevant while we're thinking about the new client API to decide if the current output format that is used in JSON is appropriate for msgpack. We may want to rethink the output format to be more friendly for chunking and consumption. I haven't thought of the best way to do that yet, but since we don't have any precedent with the msgpack encoding yet, we can likely change the output format for msgpack with no ill-effects. |
f6717ae
to
77154e8
Compare
f4d55fd
to
7b18f95
Compare
7b18f95
to
7249021
Compare
7249021
to
90236e7
Compare
90236e7
to
0ef3334
Compare
The msgpack handler correctly retains information about whether the number is a float or an integer, unlike JSON. While the format is not human-readable, it makes a good interchange format for applications that don't necessarily care about a human readable output. This uses `github.com/tinylib/msgp` to precompile the marshaling for `models.Row`. Since we only use this library to marshal the one type, this is a much more efficient method of encoding than using reflection.
0ef3334
to
a44f1a8
Compare
This will be reopened as something else. |
The msgpack handler correctly retains information about whether the
number is a float or an integer, unlike JSON. While the format is not
human-readable, it makes a good interchange format for applications that
don't necessarily care about a human readable output.