Skip to content

Request<T> and Response<T> for faster serialization #212

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.

Sign up for GitHub

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

Jump to bottom
Open
faern opened this issue Oct 29, 2017 · 2 comments
Open

Request<T> and Response<T> for faster serialization #212

faern opened this issue Oct 29, 2017 · 2 comments
Labels
idea M-server

Comments

@faern
Copy link
Contributor

faern commented Oct 29, 2017

The current Request and Response objects ultimately hold on to serde_json::Value for their actual values. This forces a user of those objects to first serialize/deserialize their types T to/from a serde_json::Value between the wire representation and the final T representation. I have not done any performance benchmarks, but I guess serializing twice does cost quite a lot of extra resources.

If these types had a generic it would be possible to have them deserialize directly from a wire format to their target type T. There would of course be some obstacles to overcome to make this work, but I think it could work. What do you think?
@tomusdrw
Copy link
Contributor

tomusdrw commented Oct 29, 2017 •
edited
Loading

Yup, make perfect sense. Probably can be even done with backward compatibility by doing Request<T: DeserializeOwned = serde_json::Value>

@faern
Copy link
Contributor Author

faern commented Oct 29, 2017 •
edited
Loading

A possible set of problems are things like Params::Array(Vec<Value>) where each item in the vector could have a different type. At the moment the way I solve it in jsonrpc-client-core is that I let the T represent the tuple of all types. so If I have a String and u64 as arguments to a method then T is (String, u64).

It basically works because tuples are serialized with [...] so it becomes a list in json. Not completely sure if this can be generalized to the Request object or not. So we would rather need something like Request<T: DeserializeOwned = jsonrpc_client_core::types::Params>

@tomusdrw tomusdrw added the idea label Jan 28, 2019
@setreadygo setreadygo mentioned this issue May 1, 2019
Open
ckamm added a commit to ckamm/jsonrpc that referenced this issue Sep 27, 2021
@ckamm
Performance: Avoid converting responses to serde_json::Value
7dd30de 
Previously, all responses were converted to serde_json::Value before
being serialized to string. Since jsonrpc does not inspect the result
object in any way, this step could be skipped.

Now, result objects are serialized to string 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 times.

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 result of rpc functions 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 paritytech#212
ckamm added a commit to ckamm/jsonrpc that referenced this issue Sep 27, 2021
@ckamm
Performance: Avoid converting responses to serde_json::Value
b2e2892 
Previously, all responses were converted to serde_json::Value before
being serialized to string. Since jsonrpc does not inspect the result
object in any way, this step could be skipped.

Now, result objects are serialized to string 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 times.

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 result of rpc functions 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 paritytech#212
@ckamm ckamm mentioned this issue Sep 27, 2021
Open
ckamm added a commit to ckamm/jsonrpc that referenced this issue Oct 24, 2021
@ckamm
Performance: Avoid converting responses to serde_json::Value
1839c50 
Previously, all responses were converted to serde_json::Value before
being serialized to string. Since jsonrpc does not inspect the result
object in any way, this step could be skipped.

Now, result objects are serialized to string 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 times.

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 result of rpc functions 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 paritytech#212
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
idea M-server
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@faern @tomusdrw