Skip to content

Debug trait and its auto implementation #7015

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

Open
wants to merge 51 commits into
base: master
Choose a base branch
from
Open

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Mar 11, 2025

Description

This PR implements the __dbg(...) intrinsic, which is very similar to Rust dbg!(...) macro.

Up until now, it has being the norm to use __log to debug values. Given that this is NOT the first use case for log, we have always found some issues with it: log does not work on predicates, log does not show "private" fields like Vec::capacity and others.

To solve these problems __dbg is being introduced:
1 - it will work on all program types, including predicates;
2 - it also prints the file name, line and column;
3 - All types will have an automatic implementation of Debug if possible, which can still be customized.
4 - Even raw_ptr and other non "loggable" types, have Debug impls.
5 - __dbg will be completely stripped in the release build by default. It can be turned on again if needed.

So this:

// Aggregates
let _ = __dbg((1u64, 2u64));
let _ = __dbg([1u64, 2u64]);

// Structs and Enums
let _ = __dbg(S { });
let _ = __dbg(E::None);
let _ = __dbg(E::Some(S { }));

will generate this:

[src/main.sw:19:13] = (1, 2)
[src/main.sw:20:13] = [1, 2]
[src/main.sw:23:13] = S { }
[src/main.sw:24:13] = None
[src/main.sw:25:13] = E(S { })

How does this work?

__dbg(value) intrinsic is desugared into { let f = Formatter{}; f.print_str(...); let value = value; value.fmt(f); value }.

Formatter is similar to Rust's one. The difference is that we still do not support string formatting, so the Formatter has a lot of print_* functions.

And each print function calls a "syscall". This syscall uses ecal under the hood and it follows unix write syscall schema.

// ssize_t write(int fd, const void buf[.count], size_t count);
fn syscall_write(fd: u64, buf: raw_ptr, count: u64) {
    asm(id: 1000, fd: fd, buf: buf, count: count) {
        ecal id fd buf count;
    }
}

For that to work, the VM interpreter must have its EcalState setup and interpret syscall number 1000 as write. This PR does this for forc test and our e2e test suite.

Each test in forc test will capture these calls and only print to the terminal when requested with the --log flag.

Garbage Collector and auto generated

Before, we were associating all auto-generated code with a pseudo file called "<autogenerated>.sw" that was never garbage collected.

This generated a problem inside the LSP when the auto_impl.rs ran a second time because of a collision in the "shareable type" map. When we try to solve this collision, choosing to keep the old value or to insert the new, the type inside the map points to already collected types and the compiler panics. This is a known problem.

The workaround for this is to break the auto-generated code into multiple files. Now they are named "main.autogenerated.sw", for example. We create one pseudo-file for each real file that needs one.

When we garbage collect one file, main.sw, for example, we also collect its associated auto-generated file.

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).
  • 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.

Copy link

codspeed-hq bot commented Mar 11, 2025

CodSpeed Performance Report

Merging #7015 will degrade performances by 34.31%

Comparing xunilrj/dbg-and-debug (e05eeee) with master (8c2af95)

Summary

❌ 4 regressions
✅ 18 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
compile 3.5 s 4.8 s -26.24%
did_change_with_caching 519.3 ms 605.1 ms -14.17%
traverse 230.7 ms 351.1 ms -34.31%
hover 3.7 ms 4.7 ms -22.57%

@xunilrj xunilrj force-pushed the xunilrj/dbg-and-debug branch from 9122060 to efa2227 Compare March 13, 2025 18:08
@xunilrj xunilrj force-pushed the xunilrj/dbg-and-debug branch from 76a644c to d49c3c8 Compare March 14, 2025 19:21
@xunilrj xunilrj self-assigned this Mar 19, 2025
@xunilrj xunilrj force-pushed the xunilrj/dbg-and-debug branch from 6c71af2 to 21c345f Compare April 15, 2025 11:07
@xunilrj
Copy link
Contributor Author

xunilrj commented Apr 15, 2025

This is the performance status after #7080

Before, the compile bench was going from 4.9s to 6.4s
image

Now is going from 3.6s to 4.8s
image

We still have the performance hit because we have a lot of more impls inside the TraitMap caused by all the auto impls, but it is faster than before.
I think we can "regain" this performance hit in future PRs.

@ironcev
Copy link
Member

ironcev commented Apr 15, 2025

I think we can "regain" this performance hit in future PRs.

I agree.

@Voxelot
Copy link
Member

Voxelot commented Apr 15, 2025

Can we just add a feature flag to disable this debug generation altogether, and that way the LSP can just disable this feature until it's more performant?

Ie. have this enabled by default, but allow the lsp to opt-out.

@JoshuaBatty
Copy link
Member

Can we just add a feature flag to disable this debug generation altogether, and that way the LSP can just disable this feature until it's more performant?

Ie. have this enabled by default, but allow the lsp to opt-out.

That's sounds like a good idea to keep performance high for LSP. We already have an optimized_build flag for this in the build config that could be used....

#[derive(Clone, Debug, Default)]
pub struct LspConfig {
    // This is set to true if compilation was triggered by a didChange LSP event. In this case, we
    // bypass collecting type metadata and skip DCA.
    //
    // This is set to false if compilation was triggered by a didSave or didOpen LSP event.
    pub optimized_build: bool,
    // The value of the `version` field in the `DidChangeTextDocumentParams` struct.
    // This is used to determine if the file has been modified since the last compilation.
    pub file_versions: BTreeMap<PathBuf, Option<u64>>,
}

@xunilrj
Copy link
Contributor Author

xunilrj commented Apr 15, 2025

Can we just add a feature flag to disable this debug generation altogether, and that way, the LSP can just disable this feature until it's more performant?

We can, but if someone uses the __dbg() intrinsics, the LSP will not compile the project. Because the type will not implement Debug, which will confuse everyone.

What we may be able to do is turn off auto-impl of the Debug trait. Implement it for primitive data-types and some types in the std library. All others must be implemented manually.

@Voxelot
Copy link
Member

Voxelot commented Apr 15, 2025

Can we just make the __dbg() intrinsic do a noop from the compilation perspective if the flag is set to disable it?

@xunilrj
Copy link
Contributor Author

xunilrj commented Apr 16, 2025

__dbg was more like an example. It will happen to any function that demands a Debug implementation. The code will not be compiled inside the LSP, but it will be using the CLI.
I will disable it for the moment, and we will accept this as a "temporary" inconvenience.

I will also look to see if there is anything else we can do for performance.

@xunilrj xunilrj dismissed stale reviews from ironcev and zees-dev via 3229229 April 16, 2025 10:47
@xunilrj xunilrj requested a review from a team as a code owner April 16, 2025 10:47
@xunilrj xunilrj force-pushed the xunilrj/dbg-and-debug branch from 3229229 to 21c345f Compare April 16, 2025 17:00
@Voxelot
Copy link
Member

Voxelot commented Apr 16, 2025

FYI @JoshuaBatty getting the selective compilation for the dbg traits working had a lot of potential footguns, and given the optimizations that @xunilrj already did in #7080 we are thinking the performance hit from this PR may not be perceivable enough on the LSP to prevent merging it.

If you give the green light for this minor hit then we can merge this pr and finish the release.

Additional Note ℹ️ : here is the regression compared the to the previous release of sway (0.67) instead of comparing to master. This is probably a more fair comparison to gauge against as it takes into account other optimization PRs that have gone in since the last release https://codspeed.io/FuelLabs/sway/runs/compare/67d2b63344ebf149d5f14115..67ffe3f48cbfc11bb7f1ff8c
image

@xunilrj xunilrj mentioned this pull request Apr 17, 2025
8 tasks
@JoshuaBatty JoshuaBatty mentioned this pull request Apr 17, 2025
3 tasks
Copy link
Member

@JoshuaBatty JoshuaBatty left a comment

Choose a reason for hiding this comment

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

Thanks for looking into the performance improvements Daniel. Very much appreciated!

@JoshuaBatty JoshuaBatty disabled auto-merge April 17, 2025 05:24
@JoshuaBatty
Copy link
Member

Disabling auto-merge to wait for #7093 to merge first as it's a prequel to this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants