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

[compiler-v2] Fixes an issue in post-processing of exp_builder #11499

Merged
merged 2 commits into from
Dec 23, 2023
Merged

Conversation

wrwg
Copy link
Contributor

@wrwg wrwg commented Dec 22, 2023

During building of expressions, placeholders are used for expressions which can't be decided because of incomplete type inference. At the end when inference is complete, a post processing phase is run to replace those placeholders. This uses the expression rewriter, but that one doesn't descend on rewrite steps.

This adds a new RewrittenAndDescend variant to rewrite functions which instructs the rewriter to descend on rewritten exps. Fortunately, this is cleanly possible because we not longer use Result as a return value.

Adds one test case which exposed this behavior from simple_map in the aptos stdlib.

Copy link

trunk-io bot commented Dec 22, 2023

⏱️ 13h 30m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-tests 2h 33m 🟥🟩🟩🟩🟩
forge-framework-upgrade-test / forge 1h 44m 🟥🟥
rust-smoke-tests 1h 26m 🟩🟩🟩
rust-move-unit-coverage 1h 19m 🟩🟩🟩🟩
windows-build 1h 16m 🟩🟩🟩🟩🟩
execution-performance / single-node-performance 58m 🟩🟩🟩
forge-compat-test / forge 43m 🟥🟩🟩
forge-e2e-test / forge 42m 🟩🟥🟩
rust-lints 37m 🟩🟩🟩🟩🟩
cli-e2e-tests / run-cli-tests 29m 🟩🟩🟩
rust-images / rust-all 27m 🟩🟩🟩
run-tests-main-branch 25m 🟩🟩🟩🟩🟩
check 20m 🟩🟩🟩🟩🟩
general-lints 14m 🟩🟩🟩🟩🟩
check-dynamic-deps 9m 🟩🟩🟩🟩🟩
node-api-compatibility-tests / node-api-compatibility-tests 3m 🟩🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩
file_change_determinator 58s 🟩🟩🟩🟩🟩
file_change_determinator 49s 🟩🟩🟩🟩🟩
file_change_determinator 34s 🟩🟩🟩
execution-performance / file_change_determinator 32s 🟩🟩🟩
execution-performance / sequential-execution-performance 21s 🟩🟩🟩
execution-performance / parallel-execution-performance 21s 🟩🟩🟩
permission-check 18s 🟩🟩🟩🟩🟩
permission-check 18s 🟩🟩🟩🟩🟩
permission-check 13s 🟩🟩🟩🟩🟩
permission-check 11s 🟩🟩🟩🟩🟩
determine-docker-build-metadata 8s 🟩🟩🟩
permission-check 5s 🟩🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
run-tests-main-branch 6m 4m +40%

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link

codecov bot commented Dec 22, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (325636f) 69.0% compared to head (e4f3ced) 69.0%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11499   +/-   ##
=======================================
  Coverage    69.0%    69.0%           
=======================================
  Files         768      768           
  Lines      178793   178794    +1     
=======================================
+ Hits       123391   123422   +31     
+ Misses      55402    55372   -30     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wrwg wrwg enabled auto-merge (squash) December 23, 2023 00:17

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wrwg added 2 commits December 22, 2023 20:01
During building of expressions, placeholders are used for expressions which can't be decided because of incomplete type inference. At the end when inference is complete, a post processing phase is run to replace those placeholders. This uses the expression rewriter, but that one doesn't descend on rewrite steps.

This adds a new `RewrittenAndDescend` variant to rewrite functions which instructs the rewriter to descend on rewritten exps. Fortunately, this is cleanly possible because we not longer use `Result` as a return value.

Adds one test case which exposed this behavior from `simple_map` in the stdlib.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.8.3 ==> e4f3ced9c9162f9a0a940de35a5e405245de1fba

Compatibility test results for aptos-node-v1.8.3 ==> e4f3ced9c9162f9a0a940de35a5e405245de1fba (PR)
1. Check liveness of validators at old version: aptos-node-v1.8.3
compatibility::simple-validator-upgrade::liveness-check : committed: 3390 txn/s, latency: 6312 ms, (p50: 6600 ms, p90: 9300 ms, p99: 11000 ms), latency samples: 189860
2. Upgrading first Validator to new version: e4f3ced9c9162f9a0a940de35a5e405245de1fba
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1825 txn/s, latency: 15868 ms, (p50: 18700 ms, p90: 22300 ms, p99: 22600 ms), latency samples: 93080
3. Upgrading rest of first batch to new version: e4f3ced9c9162f9a0a940de35a5e405245de1fba
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1784 txn/s, latency: 15785 ms, (p50: 19000 ms, p90: 21900 ms, p99: 22600 ms), latency samples: 92800
4. upgrading second batch to new version: e4f3ced9c9162f9a0a940de35a5e405245de1fba
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3484 txn/s, latency: 9023 ms, (p50: 9900 ms, p90: 13300 ms, p99: 14400 ms), latency samples: 139380
5. check swarm health
Compatibility test for aptos-node-v1.8.3 ==> e4f3ced9c9162f9a0a940de35a5e405245de1fba passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on e4f3ced9c9162f9a0a940de35a5e405245de1fba

two traffics test: inner traffic : committed: 7627 txn/s, latency: 5082 ms, (p50: 4800 ms, p90: 6300 ms, p99: 10200 ms), latency samples: 3302500
two traffics test : committed: 100 txn/s, latency: 2367 ms, (p50: 2200 ms, p90: 2600 ms, p99: 7400 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.204, avg: 0.196", "QsPosToProposal: max: 0.155, avg: 0.147", "ConsensusProposalToOrdered: max: 0.588, avg: 0.548", "ConsensusOrderedToCommit: max: 0.518, avg: 0.500", "ConsensusProposalToCommit: max: 1.069, avg: 1.048"]
Max round gap was 1 [limit 4] at version 1636241. Max no progress secs was 5.57183 [limit 10] at version 1636241.
Test Ok

@wrwg wrwg merged commit 913c716 into main Dec 23, 2023
46 of 47 checks passed
@wrwg wrwg deleted the wg-descend branch December 23, 2023 02:38
Copy link
Contributor

❌ Forge suite framework_upgrade failure on aptos-node-v1.8.3 ==> e4f3ced9c9162f9a0a940de35a5e405245de1fba

Compatibility test results for aptos-node-v1.8.3 ==> e4f3ced9c9162f9a0a940de35a5e405245de1fba (PR)
Upgrade the nodes to version: e4f3ced9c9162f9a0a940de35a5e405245de1fba
Test Failed: API error: Unknown error error sending request for url (http://aptos-node-3-validator.forge-framework-upgrade-pr-11499.svc:8080/v1/estimate_gas_price): error trying to connect: dns error: failed to lookup address information: Name or service not known

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: __libc_start_main
  14: <unknown>
Trailing Log Lines:
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: __libc_start_main
  14: <unknown>


Swarm logs can be found here: See fgi output for more information.
thread 'main' panicked at testsuite/forge/src/backend/k8s/swarm.rs:676:18:
called `Result::unwrap()` on an `Err` value: ApiError: namespaces "forge-framework-upgrade-pr-11499" not found: NotFound (ErrorResponse { status: "Failure", message: "namespaces \"forge-framework-upgrade-pr-11499\" not found", reason: "NotFound", code: 404 })

Caused by:
    namespaces "forge-framework-upgrade-pr-11499" not found: NotFound

Stack backtrace:
   0: <unknown>
   1: <unknown>
   2: <unknown>
   3: <unknown>
   4: <unknown>
   5: <unknown>
   6: <unknown>
   7: <unknown>
   8: <unknown>
   9: <unknown>
  10: <unknown>
  11: <unknown>
  12: <unknown>
  13: <unknown>
  14: __libc_start_main
  15: <unknown>
stack backtrace:
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
Debugging output:

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.

3 participants