EVM benchmark utilities #8944

sorpaas · 2018-06-21T12:00:11Z

Expose jsontests helpers from ethcore so that they can be used elsewhere. Allow hooking to start/stop point of each test so that time information can be recorded. Add a command in evmbin:

parity-evm stats-jsontests-vm <file> [--folder]

This runs executive standard jsontests against a specific folder/file, record the timing information, and then output them in tsv format. This would allow those benchmark info to be compared.

evmbin/bench.sh is moved to scripts, and a new script is added, called evm_jsontests_bench.sh, which runs the above command on all available vm jsontests.

I also changed all scripts to use /usr/bin/env instead of /bin/bash. The later would mean it breaks on all non-FHS Linux systems (like NixOS).

…nience

…rpaas/evm-jsontests

ascjones

Looks good!

dvdplm

A few grumbles, mostly on docs/discoverability.

dvdplm · 2018-06-25T06:08:12Z

ethcore/src/json_tests/chain.rs

+use super::HookType;
+
+/// Run chain jsontests on a given folder.
+pub fn run_test_path<H: FnMut(&str, HookType)>(p: &Path, skip: &[&'static str], h: &mut H) {


There are 7 identical pub fn run_test_path added in this PR – would it be possible to avoid some boilerplate and have a single copy?

Those functions are wrappers for the parent run_test_path function. Before none of them are exposed, but I figured it would be helpful to add them there, as it simplify library user's work to figure out how those functions are meant to be used.

They're not entirely identical -- the function parameter passed is different, determining which type of tests it operates on. But I'm open to any idea if we can make this better and avoid duplications. :)

dvdplm · 2018-06-25T06:10:57Z

ethcore/src/json_tests/test_common.rs

+/// Indicate the type of the hook passed to test functions.
+#[derive(Copy, Clone, Eq, PartialEq)]
+pub enum HookType {
+	/// Hook passed on starting of a test.


Perhaps this is better? "Hook to code to run on test start"

dvdplm · 2018-06-25T06:11:15Z

ethcore/src/json_tests/test_common.rs

+pub enum HookType {
+	/// Hook passed on starting of a test.
+	OnStart,
+	/// Hook passed on a test is finished.


Perhaps this is better? "Hook to code to run on test end"

dvdplm · 2018-06-25T06:12:40Z

ethcore/src/json_tests/test_common.rs

@@ -21,7 +21,20 @@ use std::path::Path;
 use std::ffi::OsString;
 pub use ethereum_types::{H256, U256, Address};

-pub fn run_test_path(p: &Path, skip: &[&'static str], runner: fn (json_data: &[u8]) -> Vec<String>) {
+/// Indicate the type of the hook passed to test functions.


Perhaps "Indicate when to run the hook passed to test functions."?

dvdplm · 2018-06-25T06:15:02Z

evmbin/src/main.rs

@@ -76,6 +77,9 @@ State test options:
    --only NAME        Runs only a single test matching the name.
    --chain CHAIN      Run only tests from specific chain.

+Stats jsontests VM options:
+    --folder           Run jsontest for a folder instead of a single file.


Could this not be inferred from the input path? If it's a file => run the file; if it's a folder => run all tests within?

dvdplm · 2018-06-25T06:16:44Z

evmbin/src/main.rs

@@ -61,6 +61,7 @@ EVM implementation for Parity.
 Usage:
    parity-evm state-test <file> [--json --std-json --only NAME --chain CHAIN]
    parity-evm stats [options]
+    parity-evm stats-jsontests-vm <file> [--folder]


The name of the command in itself does not seem sufficient to understand what this does. Not sure where the best place to put a proper description of what it does and what the output format is would be, but I think it'd be very useful.

dvdplm · 2018-06-25T06:17:29Z

evmbin/src/main.rs

+	use std::collections::HashMap;
+	use std::time::{Instant, Duration};
+
+	let file = args.arg_file.expect("FILE is required");


"FILE (or PATH) is required"?

dvdplm · 2018-06-25T06:18:17Z

evmbin/src/main.rs

+
+	let file = args.arg_file.expect("FILE is required");
+
+	let mut times: HashMap<String, (Instant, Option<Duration>)> = HashMap::new();


nitpick: I suggest calling this timings.

* master: EVM benchmark utilities (#8944)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: parity: omit redundant last imported block number in light sync informant (openethereum#8962) Disable hardware-wallets on platforms that don't support `libusb` (openethereum#8464) Bump error-chain and quick_error versions (openethereum#8972) EVM benchmark utilities (openethereum#8944) parity: hide legacy options from cli --help (openethereum#8967) scripts: fix docker build tag on latest using master (openethereum#8952) Add type for passwords. (openethereum#8920)

sorpaas added 7 commits June 21, 2018 18:08

Make it possible to expose jsontests using feature flag

4c5753e

Add start_stop_hook for jsontests

cd0c857

Fix evmbin compile

230708d

Implement vm jsontests times to tsv result

f2f8c00

Use /usr/bin/env to avoid errors on non-Debian systems

07e1807

Move evmbin/bench.sh to scripts and add vm_jsontests script for conve…

937b0d6

…nience

Merge branch 'master' of https://github.com/paritytech/parity into so…

18d3089

…rpaas/evm-jsontests

sorpaas added A0-pleasereview 🤓 Pull request needs code review. M5-binaries 📦 External binaries (ethkey, ethstore, ethvm, etc.) labels Jun 21, 2018

sorpaas added this to the 1.12 milestone Jun 21, 2018

Add tempdir as required deps for test-helpers

5a88612

ascjones approved these changes Jun 22, 2018

View reviewed changes

dvdplm reviewed Jun 25, 2018

View reviewed changes

sorpaas mentioned this pull request Jun 25, 2018

Refactor evm Instruction to be a c-like enum #8914

Merged

sorpaas added 5 commits June 25, 2018 15:22

Address grumbles on comments

17abcf5

Detect file/folder automatically and add command docs

1b30fcf

Fix bench script

477574f

times -> timings

9c8897e

typo: wrong if condition

a7d05b5

sorpaas force-pushed the sorpaas/evm-jsontests branch from 1200c22 to a7d05b5 Compare June 25, 2018 07:40

dvdplm approved these changes Jun 25, 2018

View reviewed changes

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Jun 25, 2018

ascjones approved these changes Jun 25, 2018

View reviewed changes

5chdn merged commit e9f1b38 into master Jun 25, 2018

5chdn deleted the sorpaas/evm-jsontests branch June 25, 2018 09:21

dvdplm added a commit that referenced this pull request Jun 25, 2018

Merge branch 'master' into refactor/hashdb-generic

d1ae199

* master: EVM benchmark utilities (#8944)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EVM benchmark utilities #8944

EVM benchmark utilities #8944

sorpaas commented Jun 21, 2018

ascjones left a comment

dvdplm left a comment

dvdplm Jun 25, 2018

sorpaas Jun 25, 2018

dvdplm Jun 25, 2018

dvdplm Jun 25, 2018

dvdplm Jun 25, 2018

dvdplm Jun 25, 2018

dvdplm Jun 25, 2018

dvdplm Jun 25, 2018

dvdplm Jun 25, 2018


		let file = args.arg_file.expect("FILE is required");

		let mut times: HashMap<String, (Instant, Option<Duration>)> = HashMap::new();

EVM benchmark utilities #8944

EVM benchmark utilities #8944

Conversation

sorpaas commented Jun 21, 2018

ascjones left a comment

Choose a reason for hiding this comment

dvdplm 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

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