Performance: Avoid converting responses to serde_json::Value

[ckamm]

Previously, all responses were converted to serde_json::Value before being serialized into the response string. Since jsonrpc does not inspect the result object in any way, this step could be skipped.

Now, the response string is built from the result objects much earlier and the Value step is skipped.

This patch has large performance benefits for huge responses: In tests with 4.5 GB responses, the jsonrpc serialization overhead after returning from an rpc function was reduced by around 35%. Which means several seconds of speed-up in response time.

To accomplish this while mostly retaining API compatibility, the traits RpcMethod, RpcMethodSync, RpcMethodSimple are now generic in their return type and are wrapped when added to an io handler.

There's now a distinction between the parsed representation of jsonrpc responses (Output/Response) and the result of rpc function calls (WrapOutput/WrapResponse) to allow the latter to carry the pre-serialized strings.

Review notes:

• I'm not happy with the WrapOutput / WrapResponse names, glad for suggestions.
• Similarly, rpc_wrap must be renamed and moved.
• The http handler health request is now awkward, and must extract the result/error from the already-serialized response. Probably worth it.
• The largest API breakage I could see is in the middleware, where the futures now return WrapOutput/WrapResult instead of Output/Result.
• One could make WrapOutput just be String, but having a separate type is likely easier to read.

See #212

[maciejhirsz]

You could avoid all the generics with Box<RawValue> using the to_raw_value function in place of the Value. Box<RawValue> is just a Box<str> that's guaranteed to be valid JSON, which serde_json copies as-is to whatever Serialize-able type you put it in. Compared to passing the result of the output with generics there is going to be extra allocation and memcpy (to put the JSON string of the result into the JSON string of the whole response), but that overhead might still be insignificant in the grand scheme of things.

[ckamm]

You could avoid all the generics with Box<RawValue> using the to_raw_value function in place of the Value. Box<RawValue> is just a Box<str> that's guaranteed to be valid JSON, which serde_json copies as-is to whatever Serialize-able type you put it in. Compared to passing the result of the output with generics there is going to be extra allocation and memcpy (to put the JSON string of the result into the JSON string of the whole response), but that overhead might still be insignificant in the grand scheme of things.

I had a similar idea initially, but my benchmarks showed that using RawValue instead of Value as an intermediate didn't actually save much time:

to_value+to_string: total 956ms, to_value 344ms, to_string 612ms
to_rawvalue+to_string: total 943ms, to_rawvalue 594ms, to_string 349ms
to_string: total 593ms
clone: total 338ms

This was for a 1GB result. clone is just calling String::clone on a string of that length - suggesting that this may be memory bandwidth (or more likely memory management) constrained.

[maciejhirsz]

Yeah, that would be 3GB/s for a memcpy, which sounds about right for a payload of this size on a single thread. I would have expected to_value to be more expensive (and the whole optimization to be way more than 35% cut) as it converts all structs to HashMaps, but without seeing the data you are benchmarking on it's hard to tell what's happening.

[ckamm]

Yeah, it's a very simple benchmark - can share it if you're interested. In the wild this has improved time-to-first-byte from around 26s to 19s for 4.5 GB responses. The time not spent in the actual rpc handler function has gone down from 14s to 8s. (of course having responses of that size it a problem in itself)

[ckamm]

What would need to happen to give the patch - or a variant of it - the best chances of making it into the next release that allows api changes like this?

[ckamm]

@tomusdrw Do you have thoughts on this?

If this breaks API too much and has no chance of making it in, feel free to say so. I've since seen that https://github.com/paritytech/jsonrpsee does already not have the same overhead.

[tomusdrw]

lgtm! Will probably need a major version bump.

[ckamm]

I've done the renames.

Do you have an idea for what to do about rpc_wrap(). Should I implement Into/From for the RpcMethod -> RpcMethodWithSerializedOutput conversion, or keep it explicit?

[tomusdrw]

lgtm!