Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Ensure jsonrpc threading settings are sane #11267

Merged
merged 11 commits into from
Nov 18, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Nov 15, 2019

Starting with jsonrpc v14, the "server threads" setting is more important than before and the current default of 1 means the https server is effectively single-threaded. This PR proposes a new default of 4 (and ensures that crazy settings like e.g. 0 are bumped to at least 1).

Also included: some docs, tests and cosmetics.

EDIT Thanks to @tomusdrw for pointing out that processing_threads actually has no effect. It was once used to set the size of a CpuPool, but since #9657 we use the tokio defaults: num_cpus::get() (== number of logical cores). We use the same Executor for many subsytems and if we do want to make it configurable we should expose that setting differently (and/or switch to using multiple Executors). In this PR I opted to deprecate --jsonrpc-threads and remove the corresponding processing_threads struct member, because I think the defaults are fine.

Starting with `jsonrpc` v14, the "server threads" setting is more important than before and the current default of 1 means the https server is effectively single-threaded. This PR proposes a new default of 4 (and ensures that crazy settings like e.g. `0` are bumped to at least `1`).

Also included: some docs, tests and cosmetics.
@dvdplm dvdplm self-assigned this Nov 15, 2019
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M2-config 📂 Chain specifications and node configurations. labels Nov 15, 2019
@ordian ordian added this to the 2.7 milestone Nov 15, 2019
@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Nov 15, 2019
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the changes look good, however the processing threads CLI is not used at all. It used to be used to create a separate thread pool, where RPC requests from all transports are dispatched after they are deserialized by the transport (implementation-wise it was a jsonrpc-core middleware). It seems it was lost during some refactorings.
I think we should consider bringing this back, while it doesn't have that much impact on HTTP server, it definitelly allows WS and IPC servers to process requests in parallel - currently it's going to be sequential.

parity/configuration.rs Outdated Show resolved Hide resolved
parity/rpc.rs Outdated Show resolved Hide resolved
parity/rpc.rs Outdated Show resolved Hide resolved
dvdplm and others added 2 commits November 18, 2019 06:59
Co-Authored-By: Tomasz Drwięga <[email protected]>
Co-Authored-By: Tomasz Drwięga <[email protected]>
@@ -506,18 +506,18 @@ usage! {
"--jsonrpc-hosts=[HOSTS]",
"List of allowed Host header values. This option will validate the Host header sent by the browser, it is additional security against some attack vectors. Special options: \"all\", \"none\",.",

ARG arg_jsonrpc_threads: (usize) = 4usize, or |c: &Config| c.rpc.as_ref()?.processing_threads,
ARG arg_jsonrpc_threads: (Option<usize>) = Some(4), or |c: &Config| c.rpc.as_ref()?.processing_threads,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the default is Some, should it be usize then? How a user can set it to None and does it make sense? Maybe we should keep an Option and use unwrap_or(4) instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None here implies "use defaults", i.e. the user didn't set a value. Either way, for this particular line it's moot: the jsonrpc-threads option does nothing and I removed it in dc0d349 – essentially it was once used to set the size of a CpuPool but since #9657 it has not had any effect at all.
The equivalent code today sets up the tokio Executor which is used for almost all async subsystems (sync, price fetch, rpcs and more) so it is proooobably not something users should be fiddling with. If the need to do so arises we should either use separate executors or introduce some other mechanism of configuring and reserving threads for each subsystem.

parity/rpc.rs Outdated Show resolved Hide resolved
parity/cli/mod.rs Outdated Show resolved Hide resolved
@dvdplm dvdplm requested a review from ordian November 18, 2019 11:21
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A8-looksgood 🦄 Pull request is reviewed well. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Nov 18, 2019
parity/cli/mod.rs Show resolved Hide resolved
Copy link
Collaborator

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, would just print a deprecation warning so that we can remove the parameter completely in the future.

parity/cli/mod.rs Outdated Show resolved Hide resolved
parity/deprecated.rs Outdated Show resolved Hide resolved
dvdplm and others added 3 commits November 18, 2019 16:38
Co-Authored-By: Niklas Adolfsson <[email protected]>
…thub.com:paritytech/parity-ethereum into dp/fix/better-default-jsonrpc-threading-settings

* 'dp/fix/better-default-jsonrpc-threading-settings' of github.com:paritytech/parity-ethereum:
  Update parity/deprecated.rs
@niklasad1 niklasad1 removed the A0-pleasereview 🤓 Pull request needs code review. label Nov 18, 2019
@niklasad1 niklasad1 added the A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. label Nov 18, 2019
@dvdplm dvdplm merged commit 82c3265 into master Nov 18, 2019
@dvdplm dvdplm deleted the dp/fix/better-default-jsonrpc-threading-settings branch November 18, 2019 18:40
@dvdplm dvdplm added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷🕷 labels Nov 18, 2019
dvdplm added a commit that referenced this pull request Nov 20, 2019
* master:
  Clarify what first_block `None` means (#11269)
  removed redundant VMType enum with one variant (#11266)
  Ensure jsonrpc threading settings are sane (#11267)
  Return Ok(None) when the registrar contract returns empty slice (#11257)
  Add a benchmark for snapshot::account::to_fat_rlps() (#11185)
  Fix misc compile warnings (#11258)
  simplify verification (#11249)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A7-looksgoodtestsfail 🤖 Pull request is reviewed well, but cannot be merged due to tests failing. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M2-config 📂 Chain specifications and node configurations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants