[ethash] remove mem::uninitialized #10861

ordian · 2019-07-08T11:31:11Z

Part of #10842. Supersedes #10853.

ordian · 2019-07-08T15:25:12Z

ethash/src/compute.rs

@@ -233,8 +233,7 @@ fn hash_compute(light: &Light, full_size: usize, header_hash: &H256, nonce: u64)

 			Node { bytes: out.assume_init() }
 		},
-		// This is fully initialized before being read, see `let mut compress = ...` below
-		compress_bytes: unsafe { mem::uninitialized() },
+		compress_bytes: [0u8; MIX_WORDS],


❯ cargo bench --features=bench -- compute bench_light_compute_memmap time: [980.59 us 986.10 us 992.69 us] change: [-3.7744% -1.4892% +0.5632%] (p = 0.20 > 0.05) No change in performance detected. Found 6 outliers among 100 measurements (6.00%) 4 (4.00%) high mild 2 (2.00%) high severe bench_light_compute_memory time: [1.0336 ms 1.0410 ms 1.0493 ms] change: [-3.9993% -2.0714% +0.0881%] (p = 0.05 > 0.05) No change in performance detected. Found 9 outliers among 100 measurements (9.00%)

Amazing, must have been part of one of the LLVM upgrades since this code was written. I absolutely benchmarked it before and 0-initialising it was significantly slower. Wonder if there's anywhere else where we can remove uninit entirely now.

ethash/src/compute.rs

eira-fransham

Just a single minor comment, this appears to be perfect.

eira-fransham · 2019-07-09T07:20:39Z

ethash/src/compute.rs

+
+			// This is initialized in `keccak_256` below.
+			let mut hash = mem::MaybeUninit::<[u8; 32]>::uninit();
+			keccak_256::unchecked(hash.as_mut_ptr() as *mut u8, 32, buf.as_ptr(), buf.len());


Can you extract 32 to a constant so you don't have to ensure that the 32 in the definition of hash and the 32 in the unchecked call are the same? Before you could do .len but that's now impossible because of MaybeUninit.

eira-fransham · 2019-07-09T07:27:20Z

ethash/src/compute.rs

 		// big-endian arches like mips.
 		let compress: &mut [u32; MIX_WORDS / 4] =
 			unsafe { make_const_array!(MIX_WORDS / 4, &mut buf.compress_bytes) };
+		#[cfg(target_endian = "big")]
+		{
+			compile_error!("parity-ethereum currently only supports little-endian targets");


Great, this should have been done long ago. Re-reading the code, I think it might actually work on little-endian as long as you don't copy across the files we're reading from from little- to big-endian or vice-versa, although I can't be certain. Might be worth testing.

ordian · 2019-07-09T07:45:19Z

ethash/src/compute.rs

@@ -152,9 +152,10 @@ pub fn quick_get_difficulty(header_hash: &H256, nonce: u64, mix_hash: &H256, pro

 			let buf = buf.assume_init();

+			const HASH_BYTES_LENGTH: usize = 32;


I'm bad at naming, so any other suggestions are welcome :)

Maybe KECCAK_LEN?

No, because KECCAK_LEN would be the total length right? We run keccak with different lengths during this function. At least HASH_BYTES_LEN is generic enough that it doesn't imply that we always run it with this length, it's just a barrier against typos.

* master: Run cargo fix on a few of the worst offenders (#10854) removed redundant fork choice abstraction (#10849) Extract state-db from ethcore (#10858) Fix fork choice (#10837) Move more code into state-account (#10840) Remove compiler warning (#10865) [ethash] use static_assertions crate (#10860)

ordian · 2019-07-09T08:58:03Z

Ok, so I've removed uninit completely, and running cargo bench --features=bench -- compute in ethash folder doesn't show any significant difference, but it would be nice if you could confirm this.

ordian · 2019-07-09T09:04:09Z

ethash/src/compute.rs

-			ptr::copy_nonoverlapping(&nonce as *const u64 as *const u8, buf[32..].as_mut_ptr(), 8);
+			let hash_len = header_hash.len();
+			buf[..hash_len].copy_from_slice(header_hash);
+			buf[hash_len..hash_len + mem::size_of::<u64>()].copy_from_slice(&nonce.to_ne_bytes());


I'm using here to_ne_bytes to preserve the previous behavior, but I guess it still only works for little-endian targets (don't have a machine to test this though).

dvdplm · 2019-07-09T09:33:01Z

would be nice if you could confirm this.

The benches on this branch:

     Running /Users/dvd/dev/parity/parity-ethereum/target/release/deps/basic-b975ddb423643e79
bench_light_compute_memmap
                        time:   [865.81 us 875.33 us 887.64 us]
                        change: [-7.0523% -2.5074% +1.7495%] (p = 0.30 > 0.05)
                        No change in performance detected.
Found 16 outliers among 100 measurements (16.00%)
  5 (5.00%) high mild
  11 (11.00%) high severe

bench_light_compute_memory
                        time:   [868.18 us 878.94 us 891.67 us]
                        change: [-4.1526% -1.4291% +1.4194%] (p = 0.33 > 0.05)
                        No change in performance detected.
Found 10 outliers among 100 measurements (10.00%)
  5 (5.00%) high mild
  5 (5.00%) high severe

However, the same bench command on master is… weird. Here's the output:

     Running /Users/dvd/dev/parity/parity-ethereum/target/release/deps/basic-b975ddb423643e79
bench_light_compute_memmap
                        time:   [858.62 us 869.47 us 882.64 us]
                        change: [-4.8265% -1.4632% +1.5786%] (p = 0.41 > 0.05)
                        No change in performance detected.
Found 12 outliers among 100 measurements (12.00%)
  4 (4.00%) high mild
  8 (8.00%) high severe

bench_light_compute_memmap
                        time:   [858.14 us 867.39 us 878.12 us]
                        change: [-1.7316% +0.9385% +3.9310%] (p = 0.54 > 0.05)
                        No change in performance detected.
Found 13 outliers among 100 measurements (13.00%)
  10 (10.00%) high mild
  3 (3.00%) high severe

Benchmarking bench_light_compute_memmap: Collecting 100 samples in estimated 2948.4 s (5050 iterations)^C

Notice the ~3000s estimate and the duplicate bench names.

ordian · 2019-07-09T09:53:29Z

the duplicate bench names

fixed in this PR in 5ef9344

Notice the ~3000s estimate

yeah, I guess we can force criterion to do less iterations, added 7c2d729. At this point I'm starting to think about splitting the PR into bench and non-bench part, but it's small enough so far.

* master: whisper is no longer a part of parity-ethereum repo (#10855) [ethash] remove mem::uninitialized (#10861) Docker images renaming (#10863) Move the substate module into ethcore/executive (#10867)

[ethash] replace mem::uninitialized with MaybeUninit

3cf20b1

ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Jul 8, 2019

ordian added this to the 2.7 milestone Jul 8, 2019

ordian added 3 commits July 8, 2019 15:45

[ethash] replace another occurence of mem::uninitialized

65d38f2

[ethash] clean up benches

5ef9344

[ethash] remove last mem::uninitialized

f3aff09

ordian commented Jul 8, 2019

View reviewed changes

ordian requested a review from eira-fransham July 8, 2019 15:25

[ethash] update outdated comment

23acbf3

dvdplm reviewed Jul 8, 2019

View reviewed changes

ethash/src/compute.rs Show resolved Hide resolved

[ethash] compile error on big endian targets

4cb0564

dvdplm approved these changes Jul 8, 2019

View reviewed changes

eira-fransham approved these changes Jul 9, 2019

View reviewed changes

[ethash] extract 32 into a constant

596a04b

ordian commented Jul 9, 2019

View reviewed changes

ordian added 6 commits July 9, 2019 09:55

[ethash] rename the constant to KECCAK_LEN

3700590

[ethash] bench quick_get_difficulty

c5b3d48

[ethash] remove MaybeUninit completely

ff75d3a

[ethash] replace ptr::copy_nonoverlapping with copy_from_slice

535c2f0

[ethash] s/header_len/hash_len

d446457

ordian commented Jul 9, 2019

View reviewed changes

[ethash] remove duplication in bench

fa6a39c

ordian changed the title ~~[ethash] replace mem::uninitialized with mem::MaybeUninit~~ [ethash] remove mem::uninitialized Jul 9, 2019

[ethash] add a config for basic benches

7c2d729

[ethash] fix a typo in bench fn name

4c90463

[ethash] remove needless cast

51e94fd

ordian merged commit 5a13117 into master Jul 12, 2019

ordian deleted the MaybeUninit branch July 12, 2019 08:04

ordian mentioned this pull request Jul 12, 2019

Replace all calls of mem::uninitialized with mem::MaybeUninit #10853

Closed

ordian mentioned this pull request Jan 27, 2020

Fix or allow all hard clippy errors. #11409

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ethash] remove mem::uninitialized #10861

[ethash] remove mem::uninitialized #10861

ordian commented Jul 8, 2019 •

edited

Loading

ordian Jul 8, 2019

eira-fransham Jul 9, 2019

eira-fransham left a comment

eira-fransham Jul 9, 2019

eira-fransham Jul 9, 2019

ordian Jul 9, 2019

eira-fransham Jul 9, 2019

dvdplm Jul 9, 2019

eira-fransham Jul 9, 2019 •

edited

Loading

ordian commented Jul 9, 2019

ordian Jul 9, 2019

dvdplm commented Jul 9, 2019

ordian commented Jul 9, 2019

		@@ -152,9 +152,10 @@ pub fn quick_get_difficulty(header_hash: &H256, nonce: u64, mix_hash: &H256, pro

		let buf = buf.assume_init();

		const HASH_BYTES_LENGTH: usize = 32;

[ethash] remove mem::uninitialized #10861

[ethash] remove mem::uninitialized #10861

Conversation

ordian commented Jul 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eira-fransham Jul 9, 2019 • edited Loading

Choose a reason for hiding this comment

ordian commented Jul 9, 2019

Choose a reason for hiding this comment

dvdplm commented Jul 9, 2019

ordian commented Jul 9, 2019

ordian commented Jul 8, 2019 •

edited

Loading

eira-fransham Jul 9, 2019 •

edited

Loading