Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preserve artifact cache unless stale #1918

Merged
merged 41 commits into from
Nov 19, 2023
Merged

Preserve artifact cache unless stale #1918

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
a563e25
make pruning explicit
eagr Oct 17, 2023
f31d354
preserve cache unless stale
eagr Oct 17, 2023
195bbce
barely working
eagr Oct 17, 2023
99aa012
use ArtifactId::from_file_name()
eagr Oct 17, 2023
17973ef
ignore non-unicode file names
eagr Oct 17, 2023
e81b456
generalize concat_const!()
eagr Oct 18, 2023
851a77a
per advices
eagr Oct 18, 2023
a3b0fcf
break on IO error
eagr Oct 18, 2023
b323c28
make pruning sound
eagr Oct 18, 2023
a2808f8
log more events
eagr Oct 18, 2023
0657d41
refactor
eagr Oct 20, 2023
0384ae7
doc
eagr Oct 20, 2023
4b1bb0a
Refactor indentation
mrcnski Oct 21, 2023
a8bcce4
refactor
eagr Oct 21, 2023
50b7ccc
checksum poc
eagr Oct 22, 2023
b6c1a07
Revert "checksum poc"
eagr Oct 30, 2023
2ad5262
redo checksum p1
eagr Oct 30, 2023
3723806
p2
eagr Oct 30, 2023
e44c451
remove corrupted cache
eagr Oct 31, 2023
6c19164
diversify results
eagr Nov 1, 2023
dad5285
fix tests
eagr Nov 1, 2023
1ede201
fix pruning
eagr Nov 1, 2023
2d51b52
fix message serialization
eagr Nov 1, 2023
36a33e8
clean up
eagr Nov 1, 2023
69f6a44
retire path_prefix()
eagr Nov 2, 2023
d3254f5
improve test
eagr Nov 3, 2023
f4d22fa
Merge branch 'master' into preserve-art
mrcnski Nov 12, 2023
39448ff
Fix test
mrcnski Nov 12, 2023
3a2c1cd
cargo fmt
mrcnski Nov 12, 2023
a02cb06
as per advices
eagr Nov 13, 2023
53e4557
tag artifact with runtime version
eagr Nov 13, 2023
d4f3083
fix tests
eagr Nov 13, 2023
9d2142e
Merge branch 'master' into preserve-art
mrcnski Nov 14, 2023
931aae1
upstream build fn to substrate
eagr Nov 15, 2023
5de4e8e
glitch
eagr Nov 15, 2023
a0b71c5
wrong attribution
eagr Nov 15, 2023
c85adfe
as per suggestions
eagr Nov 17, 2023
b897f1c
glitch
eagr Nov 17, 2023
89ada31
prevent `cargo tree` from accessing network
eagr Nov 17, 2023
868426a
glitch
eagr Nov 17, 2023
652bec7
Merge branch 'master' into preserve-art
eagr Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion polkadot/node/core/pvf/benches/host_prepare_rococo_runtime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ impl TestHost {
&self,
code: &[u8],
executor_params: ExecutorParams,
) -> Result<PrepareStats, PrepareError> {
) -> Result<(), PrepareError> {
let (result_tx, result_rx) = futures::channel::oneshot::channel();

let code = sp_maybe_compressed_blob::decompress(code, 16 * 1024 * 1024)
Expand Down
1 change: 1 addition & 0 deletions polkadot/node/core/pvf/common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
name = "polkadot-node-core-pvf-common"
description = "Polkadot crate that contains functionality related to PVFs that is shared by the PVF host and the PVF workers."
version = "1.0.0"
build = "build.rs"
mrcnski marked this conversation as resolved.
Show resolved Hide resolved
authors.workspace = true
edition.workspace = true
license.workspace = true
Expand Down
28 changes: 28 additions & 0 deletions polkadot/node/core/pvf/common/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
fn main() {
get_wasmtime_version();
}

pub fn get_wasmtime_version() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment that in case there are multiple wasmtime versions being used, is an error. Example:

 $ cargo tree --package=aead --depth=0
error: There are multiple `aead` packages in your project, and the specification `aead` is ambiguous.
Please re-run this command with one of the following specifications:
  [email protected]
  [email protected]
  [email protected]

I think an error in that case great, we definitely don't want to be mixing wasmtime versions anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is somehow captured. Right now this shows a warning and the compilation will fail if they try to refer to the environment variable. Would that be enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean a comment would be useful for readers of the code. I was wondering what would happen if there were multiple versions of wasmtime, I didn't know it would be an error until I tried it.

// we only care about the root of the tree
match std::process::Command::new("cargo")
.args(&["tree", "--package=wasmtime", "--depth=0"])
.output()
{
Ok(out) if out.status.success() => {
// wasmtime vX.X.X
let version = String::from_utf8_lossy(&out.stdout);
if let Some(version) = version.strip_prefix("wasmtime v") {
println!("cargo:rustc-env=SUBSTRATE_WASMTIME_VERSION={}", version);
} else {
println!("cargo:warning=build.rs: unexpected result {}", version);
}
},
Ok(out) => println!(
"cargo:warning=build.rs: `cargo tree` {}",
String::from_utf8_lossy(&out.stderr),
),
Err(err) => {
println!("cargo:warning=build.rs: Could not run `cargo tree`: {}", err);
},
}
}
2 changes: 2 additions & 0 deletions polkadot/node/core/pvf/common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ pub use sp_tracing;

const LOG_TARGET: &str = "parachain::pvf-common";

pub const RUNTIME_VERSION: &str = env!("SUBSTRATE_WASMTIME_VERSION");

use std::{
io::{self, Read, Write},
mem,
Expand Down
21 changes: 12 additions & 9 deletions polkadot/node/core/pvf/src/artifacts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@
use crate::{host::PrecheckResultSender, LOG_TARGET};
use always_assert::always;
use polkadot_core_primitives::Hash;
use polkadot_node_core_pvf_common::{error::PrepareError, prepare::PrepareStats, pvf::PvfPrepData};
use polkadot_node_core_pvf_common::{
error::PrepareError, prepare::PrepareStats, pvf::PvfPrepData, RUNTIME_VERSION,
};
use polkadot_node_primitives::NODE_VERSION;
use polkadot_parachain_primitives::primitives::ValidationCodeHash;
use polkadot_primitives::ExecutorParamsHash;
Expand Down Expand Up @@ -106,9 +108,10 @@ macro_rules! concat_const {
}}
}

const RUNTIME_PREFIX: &str = "wasmtime_";
const RUNTIME_PREFIX: &str = "wasmtime_v";
const NODE_PREFIX: &str = "polkadot_v";
const ARTIFACT_PREFIX: &str = concat_const!(RUNTIME_PREFIX, NODE_PREFIX, NODE_VERSION);
const ARTIFACT_PREFIX: &str =
concat_const!(RUNTIME_PREFIX, RUNTIME_VERSION, "_", NODE_PREFIX, NODE_VERSION);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const ARTIFACT_PREFIX: &str =
concat_const!(RUNTIME_PREFIX, RUNTIME_VERSION, "_", NODE_PREFIX, NODE_VERSION);

This is over the top. We don't need this macro, nor do we need to concat this at compile time. We are using format anyway to generate the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean we do something like this instead?

fn artifact_prefix() -> String {
	format!("{}{}_{}{}", RUNTIME_PREFIX, RUNTIME_VERSION, NODE_PREFIX, NODE_VERSION)
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the very first version. Now it still feels odd to compute the same thing over and over.. I understand this is not remotely close to hot and this isn't about performance. I understand that the macro thing may feel like too much at the first glance. When I looked at how much cleaner the code became with the const/static compared to a function call, I felt it's worth it. A const is just so much easier to use than a function, as when a value is static, it probably should be used as one. Besides, isn't it nice that drop/dealloc/format calls are wiped off from the assembly? :))

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a function you can at least put it behind a OnceCell or something. Otherwise I did a search and there are some crates that do this: const_format, concat_const, etc. But it would be sad to pull in a whole dependency for this. I wish std had this... Meh, let's just merge as-is (with the function). I'll press the green button once conflicts are resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's a function you can at least put it behind a OnceCell or something.

Function, cell, lazy static, considered all that before doing the macro as it results in the simplest interface, a static str. I won't change it back. Let's get this merged. 🚀 I'm just trying to understand why is a function call better here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just trying to understand why is a function call better here.

The problem with the macro is that it's a lot of code for readers to understand, just to declare a constant. TBH, all the options are kind of meh, but given that the code is cold, optimizing for maintainability with a plain function seems like the right call.


/// Identifier of an artifact. Encodes a code hash of the PVF and a hash of executor parameter set.
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand Down Expand Up @@ -413,7 +416,7 @@ impl Artifacts {

#[cfg(test)]
mod tests {
use super::{ArtifactId, Artifacts, ARTIFACT_PREFIX, NODE_VERSION};
use super::{ArtifactId, Artifacts, ARTIFACT_PREFIX, NODE_VERSION, RUNTIME_VERSION};
use polkadot_primitives::ExecutorParamsHash;
use rand::Rng;
use sp_core::H256;
Expand All @@ -431,10 +434,7 @@ mod tests {
}

fn file_name(code_hash: &str, param_hash: &str, checksum: &str) -> String {
format!(
"wasmtime_polkadot_v{}_0x{}_0x{}_0x{}",
NODE_VERSION, code_hash, param_hash, checksum
)
format!("{}_0x{}_0x{}_0x{}", ARTIFACT_PREFIX, code_hash, param_hash, checksum)
}

fn create_artifact(
Expand Down Expand Up @@ -481,7 +481,10 @@ mod tests {

#[test]
fn artifact_prefix() {
assert_eq!(ARTIFACT_PREFIX, format!("wasmtime_polkadot_v{}", NODE_VERSION),)
assert_eq!(
ARTIFACT_PREFIX,
format!("wasmtime_v{}_polkadot_v{}", RUNTIME_VERSION, NODE_VERSION)
);
}

#[test]
Expand Down
3 changes: 1 addition & 2 deletions polkadot/node/core/pvf/tests/it/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ use assert_matches::assert_matches;
use parity_scale_codec::Encode as _;
use polkadot_node_core_pvf::{
start, testing::get_and_check_worker_paths, Config, InvalidCandidate, Metrics, PrepareError,
PrepareJobKind, PrepareStats, PvfPrepData, ValidationError, ValidationHost,
JOB_TIMEOUT_WALL_CLOCK_FACTOR,
PrepareJobKind, PvfPrepData, ValidationError, ValidationHost, JOB_TIMEOUT_WALL_CLOCK_FACTOR,
};
use polkadot_parachain_primitives::primitives::{BlockData, ValidationParams, ValidationResult};
use polkadot_primitives::{ExecutorParam, ExecutorParams};
Expand Down
2 changes: 1 addition & 1 deletion polkadot/xcm/procedural/tests/ui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
fn ui() {
// Only run the ui tests when `RUN_UI_TESTS` is set.
if std::env::var("RUN_UI_TESTS").is_err() {
return;
mrcnski marked this conversation as resolved.
Show resolved Hide resolved
return
}

// As trybuild is using `cargo check`, we don't need the real WASM binaries.
Expand Down
4 changes: 1 addition & 3 deletions substrate/client/chain-spec/src/chain_spec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,9 +784,7 @@ fn json_eval_value_at_key(
path: &mut VecDeque<&str>,
fun: &dyn Fn(&json::Value) -> bool,
) -> bool {
let Some(key) = path.pop_front() else {
return false;
};
let Some(key) = path.pop_front() else { return false };
mrcnski marked this conversation as resolved.
Show resolved Hide resolved

if path.is_empty() {
doc.as_object().map_or(false, |o| o.get(key).map_or(false, |v| fun(v)))
Expand Down
Loading