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

refactor(fetch) : light use only one DNS thread #9647

Merged
merged 2 commits into from
Sep 28, 2018

Conversation

niklasad1
Copy link
Collaborator

Attempt to close #9623

@niklasad1 niklasad1 added A0-pleasereview 🤓 Pull request needs code review. A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Sep 25, 2018
@niklasad1 niklasad1 added the M4-core ⛓ Core client code / Rust. label Sep 25, 2018
@5chdn 5chdn added this to the 2.2 milestone Sep 25, 2018
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

The cmd.light flag is already checked here so alternatively could pass in the number of threads as a parameter in execute_impl and execute_light_impl.

thread::Builder::new().name("fetch".into()).spawn(move || {
let mut core = match reactor::Core::new() {
Ok(c) => c,
Err(e) => return tx_start.send(Err(e)).unwrap_or(())
};

let handle = core.handle();
let number_dns_threads = if is_light { 1 } else { 4 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of passing the flag down you could pass the number of threads as a parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's better, will change it!

parity/run.rs Outdated
@@ -283,7 +283,7 @@ fn execute_light_impl(cmd: RunCmd, logger: Arc<RotatingLogger>) -> Result<Runnin
let cpu_pool = CpuPool::new(4);

// fetch service
let fetch = fetch::Client::new().map_err(|e| format!("Error starting fetch client: {:?}", e))?;
let fetch = fetch::Client::new(cmd.light).map_err(|e| format!("Error starting fetch client: {:?}", e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be true so could just pass in 1 here

parity/run.rs Outdated
@@ -477,7 +477,7 @@ fn execute_impl<Cr, Rr>(cmd: RunCmd, logger: Arc<RotatingLogger>, on_client_rq:
let event_loop = EventLoop::spawn();

// fetch service
let fetch = fetch::Client::new().map_err(|e| format!("Error starting fetch client: {:?}", e))?;
let fetch = fetch::Client::new(cmd.light).map_err(|e| format!("Error starting fetch client: {:?}", e))?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be false so could just pass in 4 here

@niklasad1 niklasad1 force-pushed the refactor/fetch-one-thread-light branch from 2bb4b46 to ac3e528 Compare September 26, 2018 14:56
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 27, 2018
const VERIFY_PASSWORD_HINT: &str = "Make sure valid password is present in files passed using `--password` or in the configuration file.";

// Full client number of DNS threads
const FETCH_FULL_NUM_DNS_THREADS: usize = 4;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt we actually need all 4 for the full node either. But fine with keeping it the same for backward compatibility.

@niklasad1 niklasad1 merged commit 4784876 into master Sep 28, 2018
@niklasad1 niklasad1 deleted the refactor/fetch-one-thread-light branch September 28, 2018 13:26
dvdplm added a commit that referenced this pull request Oct 2, 2018
…mon-deps

* origin/master:
  CI: Remove unnecessary pipes (#9681)
  test.sh: use cargo --target for platforms other than linux, win or mac (#9650)
  ci: fix push script (#9679)
  Hardfork the testnets (#9562)
  Calculate sha3 instead of sha256 for push-release. (#9673)
  ethcore-io retries failed work steal (#9651)
  fix(light_fetch): avoid race with BlockNumber::Latest (#9665)
  Test fix for windows cache name... (#9658)
  refactor(fetch) : light use only one `DNS` thread (#9647)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI argument to specify the number of DNS worker threads in fetch
5 participants