Profiling feature #6565

GeorgiosDelkos · 2024-09-18T15:29:22Z

Description

This pull request adds the profile feature in the Sway compiler.

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

Checklist

I have linked to any relevant issues.
I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation where relevant (API docs, the reference, and the Sway book).
- If my change requires substantial documentation changes, I have requested support from the DevRel team
I have added tests that prove my fix is effective or that my feature works.
I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
I have requested a review from the relevant team or maintainers.

forc-pkg/Cargo.toml

alfiedotwtf · 2024-09-19T04:08:02Z

forc-pkg/src/pkg.rs

+                sway_ir::Padding::Left { target_size } => target_size,
+                sway_ir::Padding::Right { target_size } => target_size,
+            };
+


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

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.

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.

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.

This reverts commit 3b3f964.

…ze_of_val` instead of constants

CLAassistant · 2024-09-19T16:16:27Z

All committers have signed the CLA.

JoshuaBatty · 2024-09-19T23:28:13Z

forc-pkg/src/pkg.rs

+                sway_core::asm_generation::Datum::Byte(x)
+                | sway_core::asm_generation::Datum::Word(x)
+                | sway_core::asm_generation::Datum::ByteArray(x) => std::mem::size_of_val(x) as u64,
+


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

JoshuaBatty · 2024-09-19T23:30:52Z

forc-pkg/src/pkg.rs

+    #[cfg(feature = "profile")]
+    {


Can we move the code between the { } braces into a function and then call it here?

No problem, it's now been moved to the report_assembly_information function.

sdankel

Is the purpose of this profiler to profile sway code, or the rust code of the compiler itself? The name forc perf suggests the former, but it looks like this is attempting to do the latter.

Sorry if I'm missing the point here, I don't have much context on this project. Is there something that this tool does that we don't get from criterion + codspeed?

I could see the value here being that we can run it against multiple sway projects, but the maintenance of maintaining this tooling does not seem worth it. We already have a benchmark for the compile function which includes most if not all of the functions captured by time_expr. It would be good to add more benchmarks of the compile function over a diverse set of sway projects.

sdankel · 2024-09-20T21:14:48Z

sway-core/src/asm_generation/mod.rs

@@ -10,7 +10,9 @@ pub mod fuel;
 pub mod instruction_set;

 mod finalized_asm;
-pub use finalized_asm::{CompiledBytecode, FinalizedAsm, FinalizedEntry};
+pub use finalized_asm::*;
+


nit

Suggested change

sdankel · 2024-09-20T21:15:51Z

forc-pkg/Cargo.toml

+fuel-asm = { version = "0.57.0", features = ["serde"] }
+sway-ir = { version = "0.63.6", path = "../sway-ir" }


Should these dependencies only be added for the profile feature?

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.

sdankel · 2024-09-20T21:16:03Z

forc-pkg/Cargo.toml


 [dev-dependencies]
 regex = "^1.10.2"

 [target.'cfg(not(target_os = "macos"))'.dependencies]
 sysinfo = "0.29"
+
+[features]
+profile = []


nit: I'd prefer to call it profiler

sdankel · 2024-09-20T21:32:44Z

forc-pkg/src/pkg.rs

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


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?

camden-smallwood · 2024-09-20T22:09:17Z

Is the purpose of this profiler to profile sway code, or the rust code of the compiler itself? The name forc perf suggests the former, but it looks like this is attempting to do the latter.

Sorry if I'm missing the point here, I don't have much context on this project. Is there something that this tool does that we don't get from criterion + codspeed?

I could see the value here being that we can run it against multiple sway projects, but the maintenance of maintaining this tooling does not seem worth it. We already have a benchmark for the compile function which includes most if not all of the functions captured by time_expr. It would be good to add more benchmarks of the compile function over a diverse set of sway projects.

Please speak with Jibril and Nick about this, as we worked on this engagement in an external agreement with them.

You can find the forc-perf repository here: https://github.com/ourovoros-io/forc-perf

We opted to use an external profiling process so that the process-specific resource consumption is not affected by the resource consumption of the profiler itself. We added these changes in order to be able to delineate the individual phases.

IGI-111 · 2024-09-23T05:00:41Z

@sdankel

Is the purpose of this profiler to profile sway code, or the rust code of the compiler itself?

Both, or rather it's meant to help us track the performance of the compiler both in compilation time and runtime performance.

Is there something that this tool does that we don't get from criterion + codspeed?

This has been in development for longer than we've added these, but the point is to get fine grained information about the compiler execution, including memory usage in various passes, statistics of instruction usage, etc.

Codspeed is nice but it doesn't really have the level of customization we're looking for for compiler benchmarks.

I could see the value here being that we can run it against multiple sway projects

This is precisely the goal, part of the point of this is to set up a representative sample of Sway to benchmark on a standardized machine so we can track compiler performance over time.

codspeed-hq · 2024-09-23T05:15:36Z

CodSpeed Performance Report

Merging #6565 will not alter performance

_{Comparing ourovoros-io:master (bc6a73e) with master (5b7d720)}

Summary

✅ 22 untouched benchmarks

tritao · 2024-09-30T08:53:55Z

Hi @GeorgiosDelkos, this currently has some conflicts, can you get this rebased on latest master?

tritao · 2024-09-30T10:28:23Z

@sdankel The perf tool is at https://github.com/ourovoros-io/forc-perf

I've been trying to test this with the following steps:

Build Sway with this PR: cargo run --release --bin forc --features profiler
Run forc-perf: cargo run --release -- -t /dev/sway/test-case -f /dev/sway/target/release/forc

Some feedback / issues:

Usage and size metrics are mostly all reported as 0.

[
  [
    "/home/joao/dev/sway/test-case",
    {
      "cpu_usage": [
        0.0,
        0.0
      ],
      "memory_usage": [
        0.0,
        0.0
      ],
      "virtual_memory_usage": [
        0.0,
        0.0
      ],
      "disk_total_written_bytes": [
        0.0,
        0.0
      ],
      "disk_written_bytes": [
        0.0,
        0.0
      ],
      "disk_total_read_bytes": [
        0.0,
        0.0
      ],
      "disk_read_bytes": [
        0.0,
        0.0
      ],
      "bytecode_size": [
        0.0,
        0.0
      ],
      "data_section_size": [
        0.0,
        0.0
      ],
      "time": [
        -1.0,
        -7.692307692307693
      ]
    }
  ]
]

Verbose reporting even when not necessary.

On a working run, I got the following report which I find pretty verbose and just clutters the entire terminal without much useful info.

----------------------------------------------------------------------------------------------------
Metric: cpu_usage
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: memory_usage
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: virtual_memory_usage
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: disk_total_written_bytes
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: disk_written_bytes
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: disk_total_read_bytes
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: disk_read_bytes
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: bytecode_size
Previous value: 24
Current value: 24
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: datasection_size
Previous value: 0
Current value: 0
Change: 0
Percentage Change: 0%
----------------------------------------------------------------------------------------------------
----------------------------------------------------------------------------------------------------
Metric: time
Previous value: 13
Current value: 12
Change: -1
Percentage Change: -7.692307692307693%
----------------------------------------------------------------------------------------------------

No error reporting seems to happen when passing an existing folder that does not contain a Sway project.
For example, passing the src folder of a Sway project instead of the root folder containing forc.toml.
Not really an issue but the tool is kinda opaque, would be great if there was a --verbose flag that shows some output of what exactly the tool does under the hood, both for understanding and diagnostics.

tritao · 2024-10-30T10:35:15Z

@GeorgiosDelkos This still shows up some conflicts, can you please get this rebased on top of latest master?

tritao · 2024-10-30T20:08:45Z

@GeorgiosDelkos @camden-smallwood Still some issues with this, fmt check, a snapshot test failing and clippy.

camden-smallwood · 2024-10-30T21:37:52Z

@GeorgiosDelkos @camden-smallwood Still some issues with this, fmt check, a snapshot test failing and clippy.

These should be fixed now in the most recent commit.

tritao · 2024-10-30T22:49:31Z

@GeorgiosDelkos @camden-smallwood Still some issues with this, fmt check, a snapshot test failing and clippy.

These should be fixed now in the most recent commit.

Still seems to be failing on ---- should_pass/forc/help ----.

camden-smallwood · 2024-10-30T23:15:35Z

@GeorgiosDelkos @camden-smallwood Still some issues with this, fmt check, a snapshot test failing and clippy.

These should be fixed now in the most recent commit.

Still seems to be failing on ---- should_pass/forc/help ----.

I have attempted to remedy this again, but I must say that this particular test is extremely pedantic. All it's doing is checking the output of forc build -h, but the format that is in the snapshot is non-standard. If it fails again, I will have to request that someone from the Fuel team fix it instead, because the expected format does not match the output of forc build -h on my system.

camden-smallwood · 2024-10-31T16:40:11Z

After looking at this further and speaking with Jibril, I have determined that there are 2 issues with this test:

The output from the help command is formatted based on the size of the current terminal.
The options are generated using a derive macro on a Rust ABI struct. This means the layout of the struct is not stable or deterministic, which can lead to different field ordering, and in turn different output text.

I believe the help test needs to be refactored to be more deterministic, because it's quite difficult to reproduce outside of the CI.

tritao · 2024-10-31T17:07:19Z

After looking at this further and speaking with Jibril, I have determined that there are 2 issues with this test:

The output from the help command is formatted based on the size of the current terminal.

The options are generated using a derive macro on a Rust ABI struct. This means the layout of the struct is not stable or deterministic, which can lead to different field ordering, and in turn different output text.

I believe the help test needs to be refactored to be more deterministic, because it's quite difficult to reproduce outside of the CI.

Thanks for looking into this, seems like the most pragmatic solution right now is just so remove this test, opened a PR for that.

@xunilrj

## Description This test has shown itself to apparently not be deterministic, so remove it as it is causing issues when merging #6565. @xunilrj I think you added this test originally, so cc'ing you to make sure there is no issue with this. ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. Co-authored-by: Kaya Gökalp <[email protected]>

tritao · 2024-11-06T10:27:40Z

@camden-smallwood The test has now been removed. Can we get this rebased again to hopefully merge it this time?

EDIT: I was able to re-trigger the failing jobs, maybe that'll be enough?

tritao · 2024-11-06T10:59:54Z

Ok this needs a bit more work, closing this one to re-open a new PR since I have no access to the branch.

## Description This pull request adds the profile feature in the Sway compiler. It communicates with an external `forc-perf` executable in order to signal the beginning and end of different compilation phases for profiling purposes. (re-opening of #6565) ## Checklist - [x] I have linked to any relevant issues. - [x] I have commented my code, particularly in hard-to-understand areas. - [x] I have updated the documentation where relevant (API docs, the reference, and the Sway book). - [x] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [x] I have added tests that prove my fix is effective or that my feature works. - [x] I have added (or requested a maintainer to add) the necessary `Breaking*` or `New Feature` labels where relevant. - [x] I have done my best to ensure that my PR adheres to [the Fuel Labs Code Review Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md). - [x] I have requested a review from the relevant team or maintainers. --------- Co-authored-by: Camden Smallwood <[email protected]> Co-authored-by: GeorgiosDelkos <[email protected]> Co-authored-by: IGI-111 <[email protected]>

Profiling feature

3b3f964

GeorgiosDelkos requested review from a team as code owners September 18, 2024 15:29

JoshuaBatty requested changes Sep 18, 2024

View reviewed changes

forc-pkg/Cargo.toml Show resolved Hide resolved

JoshuaBatty requested review from a team September 18, 2024 22:29

JoshuaBatty assigned GeorgiosDelkos Sep 18, 2024

alfiedotwtf reviewed Sep 19, 2024

View reviewed changes

camden-smallwood added 3 commits September 19, 2024 11:46

Revert "Profiling feature"

39cff93

This reverts commit 3b3f964.

Merge remote-tracking branch 'upstream/master'

60c8291

Revert, merge upstream, undo macOS-related sysinfo changes, use `si…

4478c59

…ze_of_val` instead of constants

Use size_of_val for Datum::ByteArray, cast to u64

cdcb082

JoshuaBatty reviewed Sep 19, 2024

View reviewed changes

Move assembly information reporting to separate function

48553f0

sdankel requested changes Sep 20, 2024

View reviewed changes

Rename profile feature to profiler, clean up dependencies

f422522

IGI-111 requested review from tritao and a team September 23, 2024 05:06

IGI-111 added compiler: performance-data Everything to do with performance data generated by the compiler. performance Everything related to performance, speed wise or memory wise. labels Sep 23, 2024

Fix report_assembly_information implementation

44f7102

Rebase to master

4d78c6b

Rename : from forc-perf to dyno

525cfe1

GeorgiosDelkos requested review from Voxelot, mchristopher and luizstacio as code owners October 3, 2024 13:27

Update remove feature gating for profiling

ebc0676

tritao removed request for mchristopher, Voxelot and luizstacio October 30, 2024 10:20

tritao previously approved these changes Oct 30, 2024

View reviewed changes

Rebase to master

2a410f3

camden-smallwood dismissed tritao’s stale review via 2a410f3 October 30, 2024 16:26

Fix formatting and test cases

248098d

Attempt to fix stdout.snap again

bc6a73e

tritao mentioned this pull request Oct 31, 2024

Remove forc help test. #6682

Merged

8 tasks

tritao closed this Nov 6, 2024

tritao mentioned this pull request Nov 8, 2024

Profiling support #6705

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Profiling feature #6565

Profiling feature #6565

GeorgiosDelkos commented Sep 18, 2024

alfiedotwtf Sep 19, 2024

camden-smallwood Sep 19, 2024 •

edited

Loading

alfiedotwtf Sep 20, 2024

camden-smallwood Sep 20, 2024

CLAassistant commented Sep 19, 2024 •

edited

Loading

JoshuaBatty Sep 19, 2024

camden-smallwood Sep 20, 2024

JoshuaBatty Sep 19, 2024

camden-smallwood Sep 20, 2024

sdankel left a comment •

edited

Loading

sdankel Sep 20, 2024

sdankel Sep 20, 2024

camden-smallwood Sep 20, 2024

sdankel Sep 20, 2024

sdankel Sep 20, 2024 •

edited

Loading

camden-smallwood commented Sep 20, 2024

IGI-111 commented Sep 23, 2024 •

edited

Loading

codspeed-hq bot commented Sep 23, 2024 •

edited

Loading

tritao commented Sep 30, 2024

tritao commented Sep 30, 2024 •

edited

Loading

tritao commented Oct 30, 2024

tritao commented Oct 30, 2024

camden-smallwood commented Oct 30, 2024

tritao commented Oct 30, 2024

camden-smallwood commented Oct 30, 2024

camden-smallwood commented Oct 31, 2024

tritao commented Oct 31, 2024

tritao commented Nov 6, 2024 •

edited

Loading

tritao commented Nov 6, 2024

		fuel-asm = { version = "0.57.0", features = ["serde"] }
		sway-ir = { version = "0.63.6", path = "../sway-ir" }

Profiling feature #6565

Profiling feature #6565

Conversation

GeorgiosDelkos commented Sep 18, 2024

Description

Checklist

Choose a reason for hiding this comment

camden-smallwood Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Sep 19, 2024 • 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

sdankel left a comment • 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

sdankel Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

camden-smallwood commented Sep 20, 2024

IGI-111 commented Sep 23, 2024 • edited Loading

codspeed-hq bot commented Sep 23, 2024 • edited Loading

Merging #6565 will not alter performance

Summary

tritao commented Sep 30, 2024

tritao commented Sep 30, 2024 • edited Loading

tritao commented Oct 30, 2024

tritao commented Oct 30, 2024

camden-smallwood commented Oct 30, 2024

tritao commented Oct 30, 2024

camden-smallwood commented Oct 30, 2024

camden-smallwood commented Oct 31, 2024

tritao commented Oct 31, 2024

tritao commented Nov 6, 2024 • edited Loading

tritao commented Nov 6, 2024

camden-smallwood Sep 19, 2024 •

edited

Loading

CLAassistant commented Sep 19, 2024 •

edited

Loading

sdankel left a comment •

edited

Loading

sdankel Sep 20, 2024 •

edited

Loading

IGI-111 commented Sep 23, 2024 •

edited

Loading

codspeed-hq bot commented Sep 23, 2024 •

edited

Loading

tritao commented Sep 30, 2024 •

edited

Loading

tritao commented Nov 6, 2024 •

edited

Loading