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

net_version caches network_id to avoid redundant aquire of sync read lock #9544

Merged
merged 2 commits into from
Sep 13, 2018

Conversation

debris
Copy link
Collaborator

@debris debris commented Sep 12, 2018

issue #8746

This pull request does not resolve the underlying issue with net_version rpc method, but it is simply trying to workaround the problem. We should backport it and see if the issue with net_version still exists.

If it does, it is caused by rpc library and we should fix the problem over there.

If it does not, we should fix sync.status() locking and/or all other rpc methods that do not respond immediately

@debris debris added A0-pleasereview 🤓 Pull request needs code review. B0-patchthis 🕷 M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API. labels Sep 12, 2018
@debris debris requested review from dvdplm and tomusdrw September 12, 2018 20:14
}

fn peer_count(&self) -> Result<String> {
Ok(format!("0x{:x}", self.sync.status().num_peers as u64).to_owned())
Ok(format!("0x{:x}", self.sync.status().num_peers as u64))
Copy link
Collaborator

Choose a reason for hiding this comment

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

use {:#x} instead of 0x{:x}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably also pretty slow, we could maybe exposes a specific method to return only number of peers (best would be if it didnt require locking at all) - but it's a separate issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tomusdrw definitely, but let's fix it in a separate pr :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tomusdrw
Just so I understand this, do you mean to return like u64 instead of a String here to get rid of allocation including probably something like itoa under the hood to convert the integer to ascii/unicode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure @tomusdrw was talking about accessing sync.status() object which aquires lock :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

^^ that @niklasad1 :)

}

fn peer_count(&self) -> Result<String> {
Ok(format!("0x{:x}", self.sync.status().num_peers as u64).to_owned())
Ok(format!("0x{:x}", self.sync.status().num_peers as u64))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably also pretty slow, we could maybe exposes a specific method to return only number of peers (best would be if it didnt require locking at all) - but it's a separate issue.

@tomusdrw tomusdrw added A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 13, 2018
Copy link
Collaborator

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Let's try it, lgtm

@5chdn 5chdn merged commit 6b39131 into master Sep 13, 2018
@5chdn 5chdn deleted the net_version_cache branch September 13, 2018 08:16
@5chdn 5chdn added this to the 2.2 milestone Sep 14, 2018
@5chdn 5chdn mentioned this pull request Sep 14, 2018
21 tasks
5chdn pushed a commit that referenced this pull request Sep 14, 2018
…lock (#9544)

* net_version caches network_id to avoid redundant aquire of sync read lock, #8746

* use lower_hex display formatting for net_peerCount rpc method
@debris debris mentioned this pull request Sep 17, 2018
8 tasks
5chdn pushed a commit that referenced this pull request Sep 17, 2018
…lock (#9544)

* net_version caches network_id to avoid redundant aquire of sync read lock, #8746

* use lower_hex display formatting for net_peerCount rpc method
5chdn added a commit that referenced this pull request Sep 17, 2018
* parity-version: mark 2.0.5 track stable

* deps: bump fs-swap to 0.2.4

* Remove initial token for WS. (#9545)

* version: mark release critical

* Increase Gas-floor-target and Gas Cap (#9564)

+ Gas-floor-target increased to 8M by default

+ Gas-cap increased to 10M by default

* Improve P2P discovery (#9526)

* Add `target` to Rust traces

* network-devp2p: Don't remove discovery peer in main sync

* network-p2p: Refresh discovery more often

* Update Peer discovery protocol

* Run discovery more often when not enough nodes connected

* Start the first discovery early

* Update fast discovery rate

* Fix tests

* Fix `ping` tests

* Fixing remote Node address ; adding PingPong round

* Fix tests: update new +1 PingPong round

* Increase slow Discovery rate
Check in flight FindNode before pings

* Add `deprecated` to deprecated_echo_hash

* Refactor `discovery_round` branching

* net_version caches network_id to avoid redundant aquire of sync read lock (#9544)

* net_version caches network_id to avoid redundant aquire of sync read lock, #8746

* use lower_hex display formatting for net_peerCount rpc method
5chdn added a commit that referenced this pull request Sep 17, 2018
* parity-version: mark 2.1.0 track beta

* ci: update branch version references

* docker: release master to latest

* Fix checkpointing when creating contract failed (#9514)

* ci: fix json docs generation (#9515)

* fix typo in version string (#9516)

* Update patricia trie to 0.2.2 crates. Default dependencies on minor
version only.

* Putting back ethereum tests to the right commit

* Enable all Constantinople hard fork changes in constantinople_test.json (#9505)

* Enable all Constantinople hard fork changes in constantinople_test.json

* Address grumbles

* Remove EIP-210 activation

* 8m -> 5m

* Temporarily add back eip210 transition so we can get test passed

* Add eip210_test and remove eip210 transition from const_test

* In create memory calculation is the same for create2 because the additional parameter was popped before. (#9522)

* deps: bump fs-swap and kvdb-rocksdb

* Multithreaded snapshot creation (#9239)

* Add Progress to Snapshot Secondary chunks creation

* Use half of CPUs to multithread snapshot creation

* Use env var to define number of threads

* info to debug logs

* Add Snapshot threads as CLI option

* Randomize chunks per thread

* Remove randomness, add debugging

* Add warning

* Add tracing

* Use parity-common fix seek branch

* Fix log

* Fix tests

* Fix tests

* PR Grumbles

* PR Grumble II

* Update Cargo.lock

* PR Grumbles

* Default snapshot threads to half number of CPUs

* Fix default snapshot threads // min 1

* correct before_script for nightly build versions (#9543)

- fix gitlab array of strings syntax error
- get proper commit id
- avoid colon in stings

* Remove initial token for WS. (#9545)

* version: mark release critical

* ci: fix rpc docs generation 2 (#9550)

* Improve P2P discovery (#9526)

* Add `target` to Rust traces

* network-devp2p: Don't remove discovery peer in main sync

* network-p2p: Refresh discovery more often

* Update Peer discovery protocol

* Run discovery more often when not enough nodes connected

* Start the first discovery early

* Update fast discovery rate

* Fix tests

* Fix `ping` tests

* Fixing remote Node address ; adding PingPong round

* Fix tests: update new +1 PingPong round

* Increase slow Discovery rate
Check in flight FindNode before pings

* Add `deprecated` to deprecated_echo_hash

* Refactor `discovery_round` branching

* net_version caches network_id to avoid redundant aquire of sync read lock (#9544)

* net_version caches network_id to avoid redundant aquire of sync read lock, #8746

* use lower_hex display formatting for net_peerCount rpc method

* Increase Gas-floor-target and Gas Cap (#9564)

+ Gas-floor-target increased to 8M by default

+ Gas-cap increased to 10M by default

* Revert to old parity-tokio-ipc.

* Downgrade named pipes.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A6-mustntgrumble 💦 Pull request has areas for improvement. The author need not address them before merging. M4-core ⛓ Core client code / Rust. M6-rpcapi 📣 RPC API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants