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

[move-compiler v2] Comparison testing for file-format generator #9345

Merged
merged 4 commits into from
Jul 31, 2023
Merged

Conversation

wrwg
Copy link
Contributor

@wrwg wrwg commented Jul 27, 2023

This extends the transactional-test-runner to support compiler V2 and to do comparison testing between V1 and V2.

It ports over most of the transactional tests of the V1 compiler into the V2 tree and runs comparison testing on them, fixing bugs throughout the compilation chain as needed.

Closes #9334

NOTE: this is diffbased on #9258; only look at the latest commit.

@wrwg wrwg force-pushed the wg-txnt branch 3 times, most recently from 2673d36 to 8a5ed91 Compare July 27, 2023 20:20
@wrwg wrwg marked this pull request as ready for review July 27, 2023 20:22
wrwg added 3 commits July 27, 2023 21:27
This adds an initial version of the generation of the external representation of Move code, the so-called file format.

- Compiles from the register machine of the compiler IR into the stack machine the VM uses. Some attempts are made to optimize usage of the stack, though more can/must be done to optimize the stack machine code.
- Adds a set of basic tests, but more e2e execution tests should be added. (Subsequent PRs.)
- The translation can benefit from analysis results like live-var which allows it make smarter use of `CopyLoc` vs `MoveLoc` (to be added in the future)
@@ -0,0 +1,69 @@
comparison between v1 and v2 failed:
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you'll fix these failed tests at a later stage, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! #9343

@wrwg wrwg added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Jul 28, 2023
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

This extends the transactional-test-runner to support compiler V2 _and_ to do comparison testing between V1 and V2.

It ports over most of the transactional tests of the V1 compiler into the V2 tree and runs comparison testing on them, fixing bugs throughout the compilation chain as needed.

Closes #9334
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.5.1 ==> 9a3664783caa01ac7a03095f71f6fd209cd8a2fc

Compatibility test results for aptos-node-v1.5.1 ==> 9a3664783caa01ac7a03095f71f6fd209cd8a2fc (PR)
1. Check liveness of validators at old version: aptos-node-v1.5.1
compatibility::simple-validator-upgrade::liveness-check : committed: 4903 txn/s, latency: 6725 ms, (p50: 6600 ms, p90: 9300 ms, p99: 13600 ms), latency samples: 196140
2. Upgrading first Validator to new version: 9a3664783caa01ac7a03095f71f6fd209cd8a2fc
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1880 txn/s, latency: 15499 ms, (p50: 19700 ms, p90: 21800 ms, p99: 22300 ms), latency samples: 92140
3. Upgrading rest of first batch to new version: 9a3664783caa01ac7a03095f71f6fd209cd8a2fc
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1841 txn/s, latency: 15671 ms, (p50: 19100 ms, p90: 22000 ms, p99: 22600 ms), latency samples: 92060
4. upgrading second batch to new version: 9a3664783caa01ac7a03095f71f6fd209cd8a2fc
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3366 txn/s, latency: 9447 ms, (p50: 10000 ms, p90: 13600 ms, p99: 14200 ms), latency samples: 138040
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 9a3664783caa01ac7a03095f71f6fd209cd8a2fc passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 9a3664783caa01ac7a03095f71f6fd209cd8a2fc

two traffics test: inner traffic : committed: 5973 txn/s, latency: 6334 ms, (p50: 6000 ms, p90: 8100 ms, p99: 11000 ms), latency samples: 2676259
two traffics test : committed: 100 txn/s, latency: 3054 ms, (p50: 2900 ms, p90: 3800 ms, p99: 7200 ms), latency samples: 1800
Max round gap was 1 [limit 4] at version 1289996. Max no progress secs was 3.792645 [limit 10] at version 1289996.
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 9a3664783caa01ac7a03095f71f6fd209cd8a2fc

Compatibility test results for aptos-node-v1.5.1 ==> 9a3664783caa01ac7a03095f71f6fd209cd8a2fc (PR)
Upgrade the nodes to version: 9a3664783caa01ac7a03095f71f6fd209cd8a2fc
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 2961 txn/s, latency: 7321 ms, (p50: 7800 ms, p90: 10500 ms, p99: 11800 ms), latency samples: 165840
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 9a3664783caa01ac7a03095f71f6fd209cd8a2fc passed
Test Ok

@wrwg wrwg merged commit 53913b4 into main Jul 31, 2023
@wrwg wrwg deleted the wg-txnt branch July 31, 2023 16:22
Copy link
Contributor

@brmataptos brmataptos left a comment

Choose a reason for hiding this comment

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

Hopefully someone looked at the tests better than I.

@@ -63,3 +63,29 @@ pub fn format_diff(expected: impl AsRef<str>, actual: impl AsRef<str>) -> String
}
ret
}

pub fn format_diff_no_color(expected: impl AsRef<str>, actual: impl AsRef<str>) -> String {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is kind of redundant with the code above, but I can't simplify it much in Rust, so it's fine as is. Higher-order functions in Rust are a bit verbose.

Here's a try:

pub fn format_diff_no_color(expected: impl AsRef<str>, actual: impl AsRef<str>) -> String {
    format_diff_shared(
        expected,
        actual,
        |x: &String, ret: &mut String| {
            ret.push_str(&format!("= {}", x.replace('\n', "\n= ")));
            ret.push('\n');
        },
        |x: &String, ret: &mut String| {
            ret.push_str(&format!("+ {}", x.replace('\n', "\n+ ")));
            ret.push('\n');
        },
        |x: &String, ret: &mut String| {
            ret.push_str(&format!("- {}", x.replace('\n', "\n- ")));
            ret.push('\n');
        },
    )
}

Where the code above is changed to:


fn format_diff_shared(
    expected: impl AsRef<str>,
    actual: impl AsRef<str>,
    same_proc: impl Fn(&String, &mut String),
    add_proc: impl Fn(&String, &mut String),
    rem_proc: impl Fn(&String, &mut String),
) -> String {
    use difference::{Changeset, Difference};

    let changeset = Changeset::new(expected.as_ref(), actual.as_ref(), "\n");

    let mut ret = String::new();

    for seq in changeset.diffs {
        match &seq {
            Difference::Same(x) => {
                same_proc(x, &mut ret);
            },
            Difference::Add(x) => {
                add_proc(x, &mut ret);
            },
            Difference::Rem(x) => {
                rem_proc(x, &mut ret);
            },
        }
    }
    ret
}

pub fn format_diff(expected: impl AsRef<str>, actual: impl AsRef<str>) -> String {
    format_diff_shared(
        expected,
        actual,
        |x: &String, ret: &mut String| {
            ret.push_str(x);
            ret.push('\n');
        },
        |x: &String, ret: &mut String| {
            ret.push_str("\x1B[92m");
            ret.push_str(x);
            ret.push_str("\x1B[0m");
            ret.push('\n');
        },
        |x: &String, ret: &mut String| {
            ret.push_str("\x1B[91m");
            ret.push_str(x);
            ret.push_str("\x1B[0m");
            ret.push('\n');
        },
    )
}

gedigi pushed a commit that referenced this pull request Aug 2, 2023
* [move-compiler v2] File-format generator first version

This adds an initial version of the generation of the external representation of Move code, the so-called file format.

- Compiles from the register machine of the compiler IR into the stack machine the VM uses. Some attempts are made to optimize usage of the stack, though more can/must be done to optimize the stack machine code.
- Adds a set of basic tests, but more e2e execution tests should be added. (Subsequent PRs.)
- The translation can benefit from analysis results like live-var which allows it make smarter use of `CopyLoc` vs `MoveLoc` (to be added in the future)

* Addressing reviewer comments

* More review fixes

* [move-compiler v2] Comparison testing for file-format generator

This extends the transactional-test-runner to support compiler V2 _and_ to do comparison testing between V1 and V2.

It ports over most of the transactional tests of the V1 compiler into the V2 tree and runs comparison testing on them, fixing bugs throughout the compilation chain as needed.

Closes #9334
) {
let result = result.as_ref();
// First ensure the arguments are on the stack.
self.abstract_push_args(ctx, result);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like you're pushing twice if you happen to have something on the stack. Shouldn't this be:

if self.stack.len() != 0 {
    self.abstract_flush_stack(ctx, 0);
}
self.abstract_push_args(ctx, result.as_ref());

xbtmatt pushed a commit that referenced this pull request Aug 13, 2023
* [move-compiler v2] File-format generator first version

This adds an initial version of the generation of the external representation of Move code, the so-called file format.

- Compiles from the register machine of the compiler IR into the stack machine the VM uses. Some attempts are made to optimize usage of the stack, though more can/must be done to optimize the stack machine code.
- Adds a set of basic tests, but more e2e execution tests should be added. (Subsequent PRs.)
- The translation can benefit from analysis results like live-var which allows it make smarter use of `CopyLoc` vs `MoveLoc` (to be added in the future)

* Addressing reviewer comments

* More review fixes

* [move-compiler v2] Comparison testing for file-format generator

This extends the transactional-test-runner to support compiler V2 _and_ to do comparison testing between V1 and V2.

It ports over most of the transactional tests of the V1 compiler into the V2 tree and runs comparison testing on them, fixing bugs throughout the compilation chain as needed.

Closes #9334
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR compiler-v2
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug][compiler v2] Type inference for unbound lets
4 participants