[client] use types v2 (less alloc) #269

niklasad1 · 2021-04-12T12:59:08Z

Migrate to jsonrpsee_types::v2 which causes lots of related changes.

The major change is the client traits and related JsonRpcParam type,

enum JsonRpcParams<'a> {
    NoParams,
    Array(Vec<JsonValue>]),
    ArrayRef(&'a [JsonValue]),
    Map(BTreeMap<&'a str, JsonValue>),
}

fn request<'a>(method: &'str, params: JsonRpcParams<'a>) { .... }

The changes were easy to port to http client but with websocket client I had to major refactoring and decided to make the transport abstraction clean i.e, doesn't know anything about jsonrpc it just sends messages. Thus, the clients are responsible for constructing the messages.

re-org types::v2 module perhaps request/response or serialize/deserial or something.-

Follow up:

macro for constructing JsonRpcParams ([jsonrpsee types]: provide macro for constructing JsonRpcParams #275)
smallvec instead of Vec for batch requests.
write/port the serialize/deserialize tests for the types ([types]: jsonrpc_v2 types write serialize/deserialize tests. #276)

Closing #249 #219 #225

niklasad1 · 2021-04-13T16:16:44Z

benches/benches/bench.rs

+pub fn jsonrpsee_types_v2(crit: &mut Criterion) {
+	crit.bench_function("jsonrpsee_types_v2", |b| {
+		b.iter(|| {
+			let params: JsonRpcParams<_> = vec![&1, &2].into();


provide a macro for this:...

maybe: params_postional![1, 2, 3], params_by_key!["key" => val]?

http-client/src/transport.rs

niklasad1 · 2021-04-19T08:34:21Z

proc-macros/src/lib.rs

+			Ok(quote!(#ident::types))
+		}
+		Ok(FoundCrate::Itself) => panic!("Deriving RPC methods in any of the `jsonrpsee crates` is not supported"),
+		Err(_) => match (crate_name("jsonrpsee-http-client"), crate_name("jsonrpsee-ws-client")) {


This was a bug before we searched only for the clients crates in Cargo.toml.

So now:

Search for jsonrpsee crate

if not found search for client crates

niklasad1 · 2021-04-19T08:37:39Z

types/src/v2/error.rs

+
+/// JSONRPC error code
+#[derive(Error, Debug, PartialEq, Copy, Clone)]
+pub enum ErrorCode {


copy-pasted from old jsonrpc types with some minor tweaks.

niklasad1 · 2021-04-19T08:39:33Z

types/src/v2/mod.rs

+	Str(String),
+}
+
+impl From<SubscriptionId> for JsonValue {


avoid to call serde_json::to_value and it's infallible

…into na-client-types-v2

Manual implementation of serialize/deserialize to get rid of duplicated message string

niklasad1 · 2021-04-19T21:24:02Z

types/src/v2/error.rs

+}
+
+impl<'a> serde::Deserialize<'a> for ErrorCode {
+	fn deserialize<D>(deserializer: D) -> Result<ErrorCode, D::Error>


these are ser/deser is a little bit weird (not symmetrical) but essentially the error message can be deduced from the error code so no need to store the message as allocated string.

niklasad1 · 2021-04-19T21:37:47Z

types/src/lib.rs

+
+pub use beef::Cow;
+pub use client::*;
+pub use error::Error;


we could revisit this error type but it's quite hard to understand the different error types.

Technically we could kill the huge enum and have each client/server to have their own error type instead.

niklasad1 · 2021-04-19T21:41:56Z

types/src/v2/error.rs

+	/// JSON-RPC version.
+	pub jsonrpc: TwoPointZero,
+	/// Error.
+	pub error: ErrorCode,


@maciejhirsz I removed your type https://github.com/paritytech/jsonrpsee/blob/master/types/src/v2/mod.rs#L85-#L90. ErrorCode should be as cheap I think and more readable

niklasad1 · 2021-04-19T21:44:18Z

types/src/v2/params.rs

+#[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize)]
+#[serde(deny_unknown_fields)]
+#[serde(untagged)]
+pub enum Id {


decided to bring this back, currently we just use Number in the clients and the servers is using RawValue

but probably worth to comply with the spec

maciejhirsz

LGTM

tomusdrw · 2021-04-23T20:26:38Z

ws-client/src/client.rs

+		let curr = self.current_pending.load(Ordering::Relaxed);
+		if curr > 0 {
+			self.current_pending.store(curr - 1, Ordering::Relaxed);


There is a pretty nasty race condition here :)

niklasad1 added 8 commits April 9, 2021 14:35

rewrite me

ac8880a

v2

17bce94

PoC works without batch request

ef11d59

remove PartialEq bounds

6c57ac6

Merge remote-tracking branch 'origin/master' into na-client-types-v2

ba6dcd8

add naive benches types

7388191

misc

6765fb6

remove useless lifetime

3314df5

niklasad1 changed the title ~~[WIP]: client use types v2~~ [client] use types v2 (less alloc) Apr 13, 2021

niklasad1 requested a review from maciejhirsz April 13, 2021 16:11

niklasad1 commented Apr 13, 2021

View reviewed changes

http-client/src/transport.rs Outdated Show resolved Hide resolved

niklasad1 added 18 commits April 14, 2021 14:32

[ws client]: move request ID generation to client

7c8885b

make tests compile again

1ae28b9

[client transport]: kill leaky abstractions.

9819bb5

[http client transport]: minor changes in the API.

0b07e5d

[ws client]: fix batch requests.

ccf09cb

fix nits

9b74206

Merge remote-tracking branch 'origin/master' into na-client-types-v2

ed61ca4

[ws client]: generate two request IDs for subscrib

c1094c3

fix tests

16e4a91

remove unused types + less alloc for params.

32444b1

fix nits

0bcb54d

more tweaks.

222df5d

remove unused code

d949990

fix more nits

d298334

remove unused legacy types

ee31b7d

reorg types_v2 mod

f2ee2a2

port macros to new types

26e2425

fix tests again; more jsonvalue

1cbfba7

niklasad1 commented Apr 19, 2021

View reviewed changes

niklasad1 mentioned this pull request Apr 19, 2021

Fix clippy warning and enforce no warning in the CI #273

Merged

niklasad1 added 3 commits April 19, 2021 11:13

[types]: remove unsed dep smallvec

76d2c40

Merge branch 'na-client-types-v2' of github.com:paritytech/jsonrpsee …

552b9de

…into na-client-types-v2

rewrite me

b055e6d

dvdplm approved these changes Apr 19, 2021

View reviewed changes

niklasad1 added 3 commits April 19, 2021 23:07

[types]: error code impl ser/deser

3a7a09b

Manual implementation of serialize/deserialize to get rid of duplicated message string

[types v2]: re-org with explicit mods

bbdafb7

Merge remote-tracking branch 'origin/master' into na-client-types-v2

b922550

niklasad1 commented Apr 19, 2021

View reviewed changes

niklasad1 added 2 commits April 19, 2021 23:29

fix faulty test

6670484

add missed files

714113b

niklasad1 commented Apr 19, 2021

View reviewed changes

maciejhirsz approved these changes Apr 20, 2021

View reviewed changes

niklasad1 added 4 commits April 20, 2021 12:44

[ws client]: req_manager reserve unsubscribe slot.

6c2210d

simplify test code

6f0591a

add tracking issue for TODO

dea03d8

remove unused deps

2e5267b

niklasad1 merged commit 31153ac into master Apr 20, 2021

niklasad1 deleted the na-client-types-v2 branch April 20, 2021 12:02

tomusdrw reviewed Apr 23, 2021

View reviewed changes

niklasad1 mentioned this pull request Apr 25, 2021

[client]: client abstraction proposal #288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[client] use types v2 (less alloc) #269

[client] use types v2 (less alloc) #269

niklasad1 commented Apr 12, 2021 •

edited

Loading

niklasad1 Apr 13, 2021 •

edited

Loading

niklasad1 Apr 19, 2021

niklasad1 Apr 19, 2021

niklasad1 Apr 19, 2021

niklasad1 Apr 19, 2021

niklasad1 Apr 19, 2021

niklasad1 Apr 19, 2021

niklasad1 Apr 19, 2021 •

edited

Loading

maciejhirsz left a comment

tomusdrw Apr 23, 2021

[client] use types v2 (less alloc) #269

[client] use types v2 (less alloc) #269

Conversation

niklasad1 commented Apr 12, 2021 • edited Loading

niklasad1 Apr 13, 2021 • edited Loading

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021

Choose a reason for hiding this comment

niklasad1 Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

maciejhirsz left a comment

Choose a reason for hiding this comment

tomusdrw Apr 23, 2021

Choose a reason for hiding this comment

niklasad1 commented Apr 12, 2021 •

edited

Loading

niklasad1 Apr 13, 2021 •

edited

Loading

niklasad1 Apr 19, 2021 •

edited

Loading