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.

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

Profiling feature #6565

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
75 changes: 57 additions & 18 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions forc-pkg/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ tracing = "0.1"
url = { version = "2.2", features = ["serde"] }
vec1 = "1.8.0"
walkdir = "2"
fuel-asm = { version = "0.57.0", features = ["serde"] }
sway-ir = { version = "0.63.6", path = "../sway-ir" }
Copy link
Member

Choose a reason for hiding this comment

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

Should these dependencies only be added for the profile feature?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for pointing these out. Thefuel-asm crate is no longer being used and has been removed. The sway_ir crate is indeed only being used for the profiling functionality, so we have made this an optional dependency that only loads when using the profiler feature.


[dev-dependencies]
regex = "^1.10.2"

[target.'cfg(not(target_os = "macos"))'.dependencies]
tritao marked this conversation as resolved.
Show resolved Hide resolved
sysinfo = "0.29"

[features]
profile = []
Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd prefer to call it profiler

62 changes: 60 additions & 2 deletions forc-pkg/src/pkg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1785,6 +1785,7 @@ pub fn compile(

// First, compile to an AST. We'll update the namespace and check for JSON ABI output.
let ast_res = time_expr!(
pkg.name,
"compile to ast",
"compile_to_ast",
sway_core::compile_to_ast(
Expand Down Expand Up @@ -1823,6 +1824,7 @@ pub fn compile(
}

let asm_res = time_expr!(
pkg.name,
"compile ast to asm",
"compile_ast_to_asm",
sway_core::ast_to_asm(&handler, engines, &programs, &sway_build_config),
Expand All @@ -1837,6 +1839,7 @@ pub fn compile(
let mut program_abi = match pkg.target {
BuildTarget::Fuel => {
let program_abi_res = time_expr!(
pkg.name,
"generate JSON ABI program",
"generate_json_abi",
fuel_abi::generate_program_abi(
Expand Down Expand Up @@ -1875,6 +1878,7 @@ pub fn compile(
};

let abi = time_expr!(
pkg.name,
"generate JSON ABI program",
"generate_json_abi",
evm_abi::generate_abi_program(typed_program, engines),
Expand All @@ -1899,15 +1903,16 @@ pub fn compile(
.map(|finalized_entry| PkgEntry::from_finalized_entry(finalized_entry, engines))
.collect::<anyhow::Result<_>>()?;

let asm = match asm_res {
let mut asm = match asm_res {
Err(_) => return fail(handler),
Ok(asm) => asm,
};

let bc_res = time_expr!(
pkg.name,
"compile asm to bytecode",
"compile_asm_to_bytecode",
sway_core::asm_to_bytecode(&handler, asm, source_map, engines.se(), &sway_build_config),
sway_core::asm_to_bytecode(&handler, &mut asm, source_map, engines.se(), &sway_build_config),
Some(sway_build_config.clone()),
metrics
);
Expand Down Expand Up @@ -1957,9 +1962,62 @@ pub fn compile(
warnings,
metrics,
};

#[cfg(feature = "profile")]
report_assembly_information(&asm, &compiled_package);

Ok(compiled_package)
}

#[cfg(feature = "profile")]
fn report_assembly_information(
compiled_asm: &sway_core::CompiledAsm,
compiled_package: &CompiledPackage,
) {
let mut bytes = compiled_package.bytecode.bytes.clone();

Copy link
Contributor

Choose a reason for hiding this comment

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

To future-proof, std::mem::size_of_val can be used here instead of magic values

Copy link
Contributor

@camden-smallwood camden-smallwood Sep 19, 2024

Choose a reason for hiding this comment

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

Thanks for pointing this out. We have implemented this for the Datum::Byte, Datum::Word and Datum::ByteArray variants. The value for the other variants require the length of the data instead of the memory size of the Vec container, so we will leave these as they are.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks.

Sorry just one last thing - seeing that sum() will panic on overflow, it might be worth replacing it with something safer:

   - sway_core::asm_generation::Datum::Collection(entries) => entries.iter().map(calculate_entry_size).sum(),
   + sway_core::asm_generation::Datum::Collection(entries) => entries.iter().try_fold(0u64, |acc, &v| acc.checked_add(u64::from(v)));

And then deal with a possible error for the println!() report.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have taken a look at the data flow for the datum entries, and we have determined that it is the responsibility of the compiler to produce valid data section slot size values that sum within the range of a u64. We feel that if this value does overflow, there is an issue in the data section generation code, and a panic might actually be preferred. If the team feels that simply printing a warning would suffice, we can opt for this instead.

let data_offset = u64::from_be_bytes(bytes.iter().skip(8).take(8).cloned().collect::<Vec<_>>().try_into().unwrap());
let data_section_size = bytes.len() as u64 - data_offset;

bytes.truncate(data_offset as usize);

let mut data_section_used = 0u64;

Copy link
Member

Choose a reason for hiding this comment

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

can we remove this empty new line? Sorry for my OCD here

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

fn calculate_entry_size(entry: &sway_core::asm_generation::Entry) -> u64 {
let padding = match &entry.padding {
sway_ir::Padding::Left { target_size } => target_size,
sway_ir::Padding::Right { target_size } => target_size,
};

let value_size = match &entry.value {
sway_core::asm_generation::Datum::Byte(x) => std::mem::size_of_val(x) as u64,
sway_core::asm_generation::Datum::Word(x) => std::mem::size_of_val(x) as u64,
sway_core::asm_generation::Datum::ByteArray(x) => std::mem::size_of_val(x) as u64,
sway_core::asm_generation::Datum::Slice(slice) => slice.len() as u64,
sway_core::asm_generation::Datum::Collection(entries) => entries.iter().map(calculate_entry_size).sum(),
};

assert_eq!(*padding as u64, value_size);

value_size
}

for entry in &compiled_asm.0.data_section.value_pairs {
data_section_used += calculate_entry_size(entry);
}

let asm_information = sway_core::asm_generation::AsmInformation {
bytecode_size: bytes.len() as _,
data_section: sway_core::asm_generation::DataSectionInformation {
size: data_section_size,
used: data_section_used,
value_pairs: compiled_asm.0.data_section.value_pairs.clone(),
}
};

println!("/forc-perf info {}", serde_json::to_string(&asm_information).unwrap());
Copy link
Member

@sdankel sdankel Sep 20, 2024

Choose a reason for hiding this comment

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

It communicates with an external forc-perf executable in order to signal the beginning and end of different compilation phases for profiling purposes.

So it communicates with forc-perf over stdout? Could you add to the PR description links to forc-perf documentation, and how to test this locally?

}

impl PkgEntry {
/// Returns whether this `PkgEntry` corresponds to a test.
pub fn is_test(&self) -> bool {
Expand Down
1 change: 1 addition & 0 deletions forc/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ whoami = "1.1"

[features]
default = []
profile = []
test = []
util = []
uwu = ["uwuify"]
Expand Down
4 changes: 4 additions & 0 deletions sway-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,13 @@ thiserror = "1.0"
tracing = "0.1"
uint = "0.9"
vec1 = "1.8.0"
fuel-asm = "0.57.0"

[target.'cfg(not(target_os = "macos"))'.dependencies]
sysinfo = "0.29.0"

[features]
profile = []

[lints.clippy]
iter_over_hash_type = "deny"
Loading