Use a lock instead of atomics for snapshot Progress #11197

dvdplm · 2019-10-23T10:40:15Z

After #11178 snapshot::Progress is a bit crowded. This PR sticks the whole struct inside a RwLock.

Not very interesting code here, but this change could use an extra eye or two to ensure I got the lock scope right.

Logs, docs and todos

…napshot`returns Rename `take_at` to `request_snapshot_at` Cleanup

Fix tests

Cut down on the logging noise

* master: Pause pruning while snapshotting (#11178) Type annotation for next_key() matching of json filter options (#11192) Crypto primitives removed from ethkey (#11174) Made ecrecover implementation trait public (#11188) Remove unused macro_use. (#11191) [dependencies]: jsonrpc `14.0.1` (#11183) [receipt]: add `sender` & `receiver` to `RichReceipts` (#11179) [dependencies] bump rand 0.7 (#11022) [ethcore/builtin]: do not panic in blake2pricer on short input (#11180) TxPermissions ver 3: gas price & data (#11170) [ethash] chainspec validate `ecip1017EraRounds` non-zero (#11123) util Host: fix a double Read Lock bug in fn Host::session_readable() (#11175) ethcore client: fix a double Read Lock bug in fn Client::logs() (#11172) Aura: Report malice on sibling blocks from the same validator (#11160)

ordian

While switching to locks makes the code more readable, there are things to be aware of, like it could introduce unpredictable latencies in a hot path e.g. here and makes it harder to reason about deadlocks (e.g. double lock on read() from the same thread). But it seems to be fine in this case.

ethcore/types/src/snapshot.rs

Co-Authored-By: Andronik Ordian <[email protected]>

dvdplm · 2019-10-24T18:25:46Z

While switching to locks makes the code more readable

Interesting you think that (I don't, tbh), @ngotchac had a similar comment. Not that it matters much ofc.

I definitely agree that locks are much harder to reason about.

niklasad1 · 2019-10-25T15:42:46Z

ethcore/types/src/snapshot.rs

@@ -44,89 +43,68 @@ pub enum Snapshotting {
 #[derive(Debug)]
 pub struct Progress {
 	/// Number of accounts processed so far
-	accounts: AtomicUsize,
+	accounts: u64,


yeah, I like u64 instead usize here

niklasad1 · 2019-10-25T16:00:02Z

parity/snapshot_cmd.rs

+						let cur_size = progress.bytes();
+						if cur_size != last_size {
+							last_size = cur_size;
+							let bytes = ::informant::format_bytes(cur_size as usize);


this will truncate u64 when usize::max_value() < u64::max_value(), I don't know if it is an issue or if we just should change informant::format_bytes to take an u64 instead usize

The usizes here come from the io methods deeper down the stack that actually write bytes; they all return usize for the nr of bytes written. I wanted to be consistent and went with u64 but we can go the other way too if preferred. I don't have a strong opinion tbh.

ok, I don't have strong opinions of this but I think truncation casting deserves a comment because it is not clear to me at least whether it is intended or bug.

I went the other way in ee4a0a9 ad changed format_bytes() to take a u64 instead.

niklasad1 · 2019-10-28T08:17:01Z

parity/snapshot_cmd.rs

-					last_size = cur_size;
-					let bytes = ::informant::format_bytes(cur_size as usize);
-					info!("Snapshot: {} accounts (state), {} blocks, {} bytes", p.accounts(), p.blocks(), bytes);
+			loop {


I wonder how much slower this loop would execute w.r.t. reading a RwLock instead of AtomicUsize in each iteration?

Hopefully using a RwLock is faster, but I agree, a benchmark providing a hard number would be a good thing. The reason it's (hopefully) faster is there were several atomics getting written to with SeqCst ordering in several places, so having a single lock is hopefully faster.

Cleanup

* master: (21 commits) ropsten #6631425 foundation #8798209 (#11201) Update list of bootnodes for xDai chain (#11236) ethcore/res: add mordor testnet configuration (#11200) [chain specs]: activate `Istanbul` on mainnet (#11228) [builtin]: support `multiple prices and activations` in chain spec (#11039) Insert explicit warning into the panic hook (#11225) Snapshot restoration overhaul (#11219) Fix docker centos build (#11226) retry on gitlab system failures (#11222) Update bootnodes. (#11203) Use provided usd-per-eth value if an endpoint is specified (#11209) Use a lock instead of atomics for snapshot Progress (#11197) [informant]: `MillisecondDuration` -> `as_millis()` (#11211) Step duration map configuration parameter ported from the POA Network fork (#10902) Upgrade jsonrpc to latest (#11206) [export hardcoded sync]: use debug for `H256` (#11204) Pause pruning while snapshotting (#11178) Type annotation for next_key() matching of json filter options (#11192) Crypto primitives removed from ethkey (#11174) Made ecrecover implementation trait public (#11188) ...

dvdplm added 17 commits October 13, 2019 22:13

WIP. Typos and logging.

8f0f6d9

Format todos

e73458e

Pause pruning while a snapshot is under way

ccf3a29

Logs, docs and todos

Allocate memory for the full chunk

a436fec

Name snapshotting threads

4307695

Ensure taking_snapshot is set to false whenever and however `take_s…

382b0cc

…napshot`returns Rename `take_at` to `request_snapshot_at` Cleanup

Let "in_progress" deletion fail

4adc3e6

Fix tests

Just use an atomic

c58b30b

Review grumbles

827cfb3

Finish the sentence

1b04f40

Resolve a few todos and clarify comments.

bc7ba86

Calculate progress rate since last update

d801a73

Lockfile

20abe4a

Fix tests

cf0c89f

typo

3fcb56c

Reinstate default snapshotting frequency

f8e1c42

Cut down on the logging noise

Use a lock instead of atomics for snapshot Progress

1e26644

dvdplm self-assigned this Oct 23, 2019

dvdplm added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Oct 23, 2019

dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. labels Oct 24, 2019

dvdplm marked this pull request as ready for review October 24, 2019 17:58

dvdplm added A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). M4-core ⛓ Core client code / Rust. and removed A2-insubstantial 👶 Pull request requires no code review (e.g., a sub-repository hash update). labels Oct 24, 2019

ordian approved these changes Oct 24, 2019

View reviewed changes

ethcore/types/src/snapshot.rs Outdated Show resolved Hide resolved

ordian added this to the 2.7 milestone Oct 24, 2019

Update ethcore/types/src/snapshot.rs

44b5d33

Co-Authored-By: Andronik Ordian <[email protected]>

niklasad1 reviewed Oct 25, 2019

View reviewed changes

niklasad1 reviewed Oct 28, 2019

View reviewed changes

niklasad1 approved these changes Oct 28, 2019

View reviewed changes

Avoid truncating cast

ee4a0a9

Cleanup

dvdplm merged commit 0d3423c into master Oct 28, 2019

dvdplm deleted the dp/chore/put-snapshot-progress-behind-rwlock branch October 28, 2019 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a lock instead of atomics for snapshot Progress #11197

Use a lock instead of atomics for snapshot Progress #11197

dvdplm commented Oct 23, 2019 •

edited

Loading

ordian left a comment

dvdplm commented Oct 24, 2019

niklasad1 Oct 25, 2019

niklasad1 Oct 25, 2019 •

edited

Loading

dvdplm Oct 25, 2019

niklasad1 Oct 28, 2019

dvdplm Oct 28, 2019 •

edited

Loading

niklasad1 Oct 28, 2019

dvdplm Oct 28, 2019

Use a lock instead of atomics for snapshot Progress #11197

Use a lock instead of atomics for snapshot Progress #11197

Conversation

dvdplm commented Oct 23, 2019 • edited Loading

ordian left a comment

Choose a reason for hiding this comment

dvdplm commented Oct 24, 2019

niklasad1 Oct 25, 2019

Choose a reason for hiding this comment

niklasad1 Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

dvdplm Oct 25, 2019

Choose a reason for hiding this comment

niklasad1 Oct 28, 2019

Choose a reason for hiding this comment

dvdplm Oct 28, 2019 • edited Loading

Choose a reason for hiding this comment

niklasad1 Oct 28, 2019

Choose a reason for hiding this comment

dvdplm Oct 28, 2019

Choose a reason for hiding this comment

dvdplm commented Oct 23, 2019 •

edited

Loading

niklasad1 Oct 25, 2019 •

edited

Loading

dvdplm Oct 28, 2019 •

edited

Loading