Move Throughput into `sc-sysinfo` #12368

Szegoo · 2022-09-27T15:59:21Z

Prepares for #12017

Polkadot address: 126X27SbhrV19mBFawys3ovkyBS87SGfYwtwa8J2FjHrtbmA

ggwpez · 2022-09-28T20:11:04Z

I think it is also fine to split the issue into two MRs, depending on your preferences.
The changes here already make sense to have on their own.

Szegoo · 2022-09-28T20:14:28Z

I think it is also fine to split the issue into two MRs, depending on your preferences. The changes here already make sense to have on their own.

Sure, that makes sense.

ggwpez · 2022-09-28T20:18:58Z

I can review earliest on Friday. But maybe @koute wants to, it touches your code :)

You are only reafactoring code as preparation for #12017, right?
Then please update the issue description to not Close the issue.
We also need to ensure that this does not break telemetry.

Szegoo · 2022-09-28T20:22:09Z

You are only reafactoring code as preparation for #12017, right? Then please update the issue description to not Close the issue. We also need to ensure that this does not break telemetry.

Yes, this is only preparation for #12017, I changed the description now. The CI doesn't seem happy, I will do some minor fixes on this.

client/sysinfo/src/lib.rs

bkchr · 2022-09-28T21:11:55Z

I also don't really get why we need Throughput there. @ggwpez can you give more context?

client/sysinfo/src/sysinfo.rs

ggwpez · 2022-09-29T14:32:44Z

I also don't really get why we need Throughput there. @ggwpez can you give more context?

The measurement of the performance at startup runs in sc-sysinfo. So if we now want to compare the results of that to anything, we need the Throughput types to have nicer compare logic and formatting in the final output. The type only exists to make it possible to deserialize from JSON with a unit suffix.
I just originally did not put it in sy-sysinfo to keep the sc- crate small, since it was not needed.

client/sysinfo/src/sysinfo.rs

utils/frame/benchmarking-cli/src/machine/hardware.rs

client/sysinfo/src/sysinfo.rs

client/sysinfo/src/lib.rs

bkchr · 2022-10-20T09:28:00Z

utils/frame/benchmarking-cli/src/machine/hardware.rs

@@ -45,6 +101,7 @@ pub struct Requirement {
 	/// The metric to measure.
 	pub metric: Metric,
 	/// The minimal throughput that needs to be archived for this requirement.
+	#[serde(serialize_with = "serialize_throughput", deserialize_with = "deserialize_throughput")]


Why do we need to support deserialize at all? @ggwpez?

Ahh, now I see. Do we really need to serialize with the unit attached? I don't think we should do this. You can just serialize as mib and fine.

I don't think we should do this, the reference_hardware.json has values specified in other units also, of course, we can probably change that, but isn't it nicer to have the biggest possible unit used? @bkchr

Yea the only reason to have units was the maintainability of that file. Although if we add an option to auto-generate it, then it should be fine either way.

I mean normally you would just do it in bytes and fine. Like having there Mibs etc also requires documentation, as no one knows which kind of units are supported.

bkchr · 2022-10-20T09:58:17Z

utils/frame/benchmarking-cli/src/machine/hardware.rs

+	let mut map = serializer.serialize_map(Some(1))?;
+	let (value, unit) = t.normalize();
+	let unit_as_str = match unit {
+		Unit::GiBs => "GiBs",
+		Unit::MiBs => "MiBs",
+		Unit::KiBs => "KiBs",
+	};
+
+	map.serialize_entry(&unit_as_str, &value)?;
+	map.end()


Suggested change

let mut map = serializer.serialize_map(Some(1))?;

let (value, unit) = t.normalize();

let unit_as_str = match unit {

Unit::GiBs => "GiBs",

Unit::MiBs => "MiBs",

Unit::KiBs => "KiBs",

};

map.serialize_entry(&unit_as_str, &value)?;

map.end()

let (value, unit) = t.normalize();

serializer.serialize_str(&format!("{} {}", value, unit))

@bkchr I explicitly requested that this will be done the way it is now. (:

See these comments why:

#12368 (comment)
#12368 (comment)

Maybe a comment in the code would be good why this match is currently duplicated? Or just don't serialize/deserialize the units at all and keep it all in MB/s (but that's up to @ggwpez).

Same argument as above - just the maintainability of the json file. Since having units is self explanatory, otherwise it needs docs to explain what it means.

Hmm okay, I'm not really convinced of having the mibs etc be part of the json. However, I also don't really care. If @ggwpez is happy, he can approve and I will not complain.

ggwpez

I dont have a strong opinion on the serialization stuff.

client/sysinfo/src/sysinfo.rs

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

Szegoo · 2022-10-20T15:48:03Z

@ggwpez So what is the final decision then?

ggwpez · 2022-10-20T17:52:55Z

@ggwpez So what is the final decision then?

Okay then lets remove the unit from the serialization. Less code - less bugs.

Szegoo · 2022-10-20T17:58:24Z

Okay then lets remove the unit from the serialization. Less code - less bugs.

@ggwpez What should we do with reference_hardware.json then?

Edit: wouldn't auto-generating it result in even more code?

ggwpez · 2022-10-29T08:28:12Z

@ggwpez What should we do with reference_hardware.json then?

Change it so that it works with the new format.

Edit: wouldn't auto-generating it result in even more code?

We need auto-generation at one point anyway, as manual editing is error-prone and not straight forward.

Szegoo · 2022-10-29T12:46:10Z

@ggwpez So I assume we should use MiBs in the reference_hardware.json then?

bkchr · 2022-11-03T22:48:34Z

@ggwpez can we merge this?

utils/frame/benchmarking-cli/src/machine/hardware.rs

ggwpez · 2022-11-04T16:45:26Z

bot merge

paritytech-processbot · 2022-11-04T16:45:34Z

Waiting for commit status.

Szegoo added 2 commits September 27, 2022 17:58

move Throughput to sc-sysinfo

ada0ee0

replace u64

d25f110

Szegoo marked this pull request as ready for review September 28, 2022 20:14

Szegoo changed the title ~~Warn validators with slow hardware~~ Move Throughput into sc-sysinfo Sep 28, 2022

Merge branch 'paritytech:master' into hardware-bench

8f7084e

bkchr reviewed Sep 28, 2022

View reviewed changes

client/sysinfo/src/lib.rs Outdated Show resolved Hide resolved

fix in tests

a564d4f

koute reviewed Sep 29, 2022

View reviewed changes

client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved

change Throughput

7c30f8d

Szegoo commented Sep 29, 2022

View reviewed changes

client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved

Szegoo added 2 commits September 29, 2022 19:20

refactored Throughput

d276670

fixes

96ec3f0

koute reviewed Sep 30, 2022

View reviewed changes

client/sysinfo/src/sysinfo.rs Show resolved Hide resolved

client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved

utils/frame/benchmarking-cli/src/machine/hardware.rs Show resolved Hide resolved

moved tests & fixes

7226a57

Szegoo commented Sep 30, 2022

View reviewed changes

client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved

Szegoo added 6 commits September 30, 2022 17:32

custom serializer

f5530ac

note

99b25a7

fix serializer

cdd84d2

forgot to remove

9092ba0

deserialize

621b56c

functioning deserialization :)

5f256b0

koute reviewed Oct 1, 2022

View reviewed changes

client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved

koute reviewed Oct 1, 2022

View reviewed changes

client/sysinfo/src/lib.rs Outdated Show resolved Hide resolved

try to make clipply happy

5731506

Szegoo requested review from ggwpez and bkchr and removed request for ggwpez October 19, 2022 10:19

bkchr reviewed Oct 20, 2022

View reviewed changes

Szegoo force-pushed the hardware-bench branch from daefc55 to cbca2c2 Compare October 20, 2022 10:09

Merge branch 'paritytech:master' into hardware-bench

698d53e

ggwpez approved these changes Oct 20, 2022

View reviewed changes

client/sysinfo/src/sysinfo.rs Outdated Show resolved Hide resolved

Update client/sysinfo/src/sysinfo.rs

30fe5ee

Co-authored-by: Oliver Tale-Yazdi <[email protected]>

Szegoo and others added 3 commits October 29, 2022 15:04

remove unit from serialization

7a3fe3e

Merge branch 'paritytech:master' into hardware-bench

70d48ba

Merge branch 'paritytech:master' into hardware-bench

3590c83

ggwpez reviewed Nov 4, 2022

View reviewed changes

utils/frame/benchmarking-cli/src/machine/hardware.rs Outdated Show resolved Hide resolved

Update utils/frame/benchmarking-cli/src/machine/hardware.rs

9211e14

paritytech-processbot bot merged commit 6bd2eac into paritytech:master Nov 4, 2022

ggwpez mentioned this pull request Nov 7, 2022

Machine metrics: remove duplicate units #12634

Merged

aurexav mentioned this pull request Jan 10, 2023

To polkadot-v0.9.33 darwinia-network/darwinia-2.0#194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Throughput into `sc-sysinfo` #12368

Move Throughput into `sc-sysinfo` #12368

Szegoo commented Sep 27, 2022 •

edited

Loading

ggwpez commented Sep 28, 2022 •

edited

Loading

Szegoo commented Sep 28, 2022

ggwpez commented Sep 28, 2022

Szegoo commented Sep 28, 2022

bkchr commented Sep 28, 2022

ggwpez commented Sep 29, 2022 •

edited

Loading

bkchr Oct 20, 2022

bkchr Oct 20, 2022

Szegoo Oct 20, 2022 •

edited

Loading

ggwpez Oct 20, 2022

bkchr Oct 20, 2022

bkchr Oct 20, 2022

koute Oct 20, 2022

ggwpez Oct 20, 2022

bkchr Oct 20, 2022

ggwpez left a comment

Szegoo commented Oct 20, 2022

ggwpez commented Oct 20, 2022 •

edited

Loading

Szegoo commented Oct 20, 2022 •

edited

Loading

ggwpez commented Oct 29, 2022

Szegoo commented Oct 29, 2022 •

edited

Loading

bkchr commented Nov 3, 2022

ggwpez commented Nov 4, 2022

paritytech-processbot bot commented Nov 4, 2022

Move Throughput into sc-sysinfo #12368

Move Throughput into sc-sysinfo #12368

Conversation

Szegoo commented Sep 27, 2022 • edited Loading

ggwpez commented Sep 28, 2022 • edited Loading

Szegoo commented Sep 28, 2022

ggwpez commented Sep 28, 2022

Szegoo commented Sep 28, 2022

bkchr commented Sep 28, 2022

ggwpez commented Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Szegoo Oct 20, 2022 • edited Loading

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

ggwpez left a comment

Choose a reason for hiding this comment

Szegoo commented Oct 20, 2022

ggwpez commented Oct 20, 2022 • edited Loading

Szegoo commented Oct 20, 2022 • edited Loading

ggwpez commented Oct 29, 2022

Szegoo commented Oct 29, 2022 • edited Loading

bkchr commented Nov 3, 2022

ggwpez commented Nov 4, 2022

paritytech-processbot bot commented Nov 4, 2022

Move Throughput into `sc-sysinfo` #12368

Move Throughput into `sc-sysinfo` #12368

Szegoo commented Sep 27, 2022 •

edited

Loading

ggwpez commented Sep 28, 2022 •

edited

Loading

ggwpez commented Sep 29, 2022 •

edited

Loading

Szegoo Oct 20, 2022 •

edited

Loading

ggwpez commented Oct 20, 2022 •

edited

Loading

Szegoo commented Oct 20, 2022 •

edited

Loading

Szegoo commented Oct 29, 2022 •

edited

Loading