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] Track live var info in spec blocks by default #14890

Merged
merged 2 commits into from
Oct 15, 2024

Conversation

rahxephon89
Copy link
Contributor

@rahxephon89 rahxephon89 commented Oct 8, 2024

Description

Currently, live var analysis only tracks the live var info in specs when track_all_usages is set. This leads to the issue in #14762 because variables that appear in spec block but are not alive anymore will not be dropped in the generated bytecode. This PR resolves the issue by tracking the live var info by default.

Close #14762

How Has This Been Tested?

  1. Existing tests pass;
  2. Add tests for compiler-v2 and prover.

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Other (specify)

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Oct 8, 2024

⏱️ 5h 13m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 24m 🟩
dispatch_event 16m 🟩
rust-cargo-deny 16m 🟩🟩🟩🟩🟩 (+4 more)
dispatch_event 16m 🟩
rust-move-unit-coverage 15m 🟩
dispatch_event 15m 🟩
rust-move-unit-coverage 15m 🟩
dispatch_event 15m 🟩
check-dynamic-deps 11m 🟩🟩🟩🟩🟩 (+4 more)
rust-move-unit-coverage 11m 🟩
rust-move-unit-coverage 11m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩
rust-move-tests 10m 🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rahxephon89 and the rest of your teammates on Graphite Graphite

@rahxephon89 rahxephon89 changed the title block metadata txn api update (#14848) [WIP] Fix 14762 Oct 8, 2024
@rahxephon89 rahxephon89 changed the title [WIP] Fix 14762 [WIP][Compiler-v2] Remove locals that are no longer alive after an inline spec in file-format generator Oct 8, 2024
Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.1%. Comparing base (2a0e7d6) to head (651f2de).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
...iler-v2/src/pipeline/livevar_analysis_processor.rs 50.0% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main   #14890       +/-   ##
===========================================
- Coverage    71.5%    60.1%    -11.5%     
===========================================
  Files        2409      856     -1553     
  Lines      490368   211035   -279333     
===========================================
- Hits       350832   126896   -223936     
+ Misses     139536    84139    -55397     

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

@rahxephon89 rahxephon89 force-pushed the teng/fix-14762 branch 3 times, most recently from 4a5545f to c755a04 Compare October 8, 2024 04:25
@rahxephon89 rahxephon89 changed the title [WIP][Compiler-v2] Remove locals that are no longer alive after an inline spec in file-format generator \[Compiler-v2] Remove locals that are no longer alive after an inline spec in file-format generator Oct 8, 2024
@rahxephon89 rahxephon89 changed the title \[Compiler-v2] Remove locals that are no longer alive after an inline spec in file-format generator [Compiler-v2] Remove locals that are no longer alive after an inline spec in file-format generator Oct 8, 2024
@rahxephon89 rahxephon89 marked this pull request as ready for review October 8, 2024 05:24
@rahxephon89 rahxephon89 marked this pull request as draft October 8, 2024 20:25
@rahxephon89 rahxephon89 force-pushed the teng/fix-14762 branch 3 times, most recently from 0bf4cb6 to e551142 Compare October 10, 2024 07:50
@rahxephon89 rahxephon89 changed the title [Compiler-v2] Remove locals that are no longer alive after an inline spec in file-format generator [Compiler-v2] Track live var info in specs by default Oct 10, 2024
@@ -1,64 +1,4 @@
Move prover returns: exiting with verification errors
warning: Unused assignment to `x0`. Consider removing or prefixing with an underscore: `_x0`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Warnings are removed because these variables appear in the spec, which is consistent with the behavior of the prover when using V1 compiler.

@rahxephon89 rahxephon89 changed the title [Compiler-v2] Track live var info in specs by default [Compiler-v2] Track live var info in spec blocks by default Oct 10, 2024
@rahxephon89 rahxephon89 marked this pull request as ready for review October 10, 2024 17:21
@@ -373,12 +371,12 @@ impl<'a> TransferFunctions for LiveVarAnalysis<'a> {
Branch(id, _, _, src) => {
state.insert_or_update(*src, self.livevar_info(id, offset), self.track_all_usages);
},
Prop(id, _, exp) if self.track_all_usages => {
Prop(id, _, exp) => {
for idx in exp.used_temporaries() {
state.insert_or_update(idx, self.livevar_info(id, offset), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
state.insert_or_update(idx, self.livevar_info(id, offset), true);
state.insert_or_update(idx, self.livevar_info(id, offset), self.track_all_usages);

Copy link
Contributor

Choose a reason for hiding this comment

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

This would keep it up with how self.track_all_usages are used elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

for idx in exp.used_temporaries() {
state.insert_or_update(idx, self.livevar_info(id, offset), true);
}
},
SpecBlock(id, spec) if self.track_all_usages => {
SpecBlock(id, spec) => {
for idx in spec.used_temporaries() {
state.insert_or_update(idx, self.livevar_info(id, offset), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
state.insert_or_update(idx, self.livevar_info(id, offset), true);
state.insert_or_update(idx, self.livevar_info(id, offset), self.track_all_usages);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

┌─ tests/sources/functional/bug_14762.move:45:26
39 │ let (found, index) = find(&s.entries, |obj| {
│ ---------- It is still being borrowed by this reference
Copy link
Contributor

Choose a reason for hiding this comment

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

This error seems overly conservative. Shouldn't this reference &s.entries not outlive the call to find ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is generated by V1 compiler. Do we want to take care of this?

@rahxephon89 rahxephon89 enabled auto-merge (squash) October 15, 2024 22:49

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7

two traffics test: inner traffic : committed: 13532.55 txn/s, latency: 2939.83 ms, (p50: 2700 ms, p70: 3000, p90: 3300 ms, p99: 3900 ms), latency samples: 5145360
two traffics test : committed: 100.09 txn/s, latency: 2502.42 ms, (p50: 2400 ms, p70: 2600, p90: 2800 ms, p99: 6900 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.238, avg: 0.223", "QsPosToProposal: max: 0.298, avg: 0.263", "ConsensusProposalToOrdered: max: 0.325, avg: 0.305", "ConsensusOrderedToCommit: max: 0.491, avg: 0.467", "ConsensusProposalToCommit: max: 0.799, avg: 0.772"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.05s no progress at version 2532970 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.09s no progress at version 2532966 (avg 8.09s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 7eeba4cd15892717741a614add1afde004c7855f ==> 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7

Compatibility test results for 7eeba4cd15892717741a614add1afde004c7855f ==> 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7 (PR)
Upgrade the nodes to version: 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1093.17 txn/s, submitted: 1093.91 txn/s, failed submission: 0.74 txn/s, expired: 0.74 txn/s, latency: 3011.19 ms, (p50: 2400 ms, p70: 3000, p90: 5700 ms, p99: 8700 ms), latency samples: 88740
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1077.62 txn/s, submitted: 1079.21 txn/s, failed submission: 1.59 txn/s, expired: 1.59 txn/s, latency: 2860.21 ms, (p50: 2400 ms, p70: 2700, p90: 5500 ms, p99: 6700 ms), latency samples: 95020
5. check swarm health
Compatibility test for 7eeba4cd15892717741a614add1afde004c7855f ==> 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7 passed
Upgrade the remaining nodes to version: 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1039.15 txn/s, submitted: 1040.31 txn/s, failed submission: 1.16 txn/s, expired: 1.16 txn/s, latency: 3076.01 ms, (p50: 2700 ms, p70: 3600, p90: 5000 ms, p99: 7500 ms), latency samples: 89360
Test Ok

Copy link
Contributor

✅ Forge suite compat success on 7eeba4cd15892717741a614add1afde004c7855f ==> 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7

Compatibility test results for 7eeba4cd15892717741a614add1afde004c7855f ==> 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7 (PR)
1. Check liveness of validators at old version: 7eeba4cd15892717741a614add1afde004c7855f
compatibility::simple-validator-upgrade::liveness-check : committed: 13371.21 txn/s, latency: 2379.01 ms, (p50: 2000 ms, p70: 2200, p90: 3000 ms, p99: 9600 ms), latency samples: 515900
2. Upgrading first Validator to new version: 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5301.39 txn/s, latency: 5354.20 ms, (p50: 5900 ms, p70: 6200, p90: 7000 ms, p99: 7100 ms), latency samples: 112100
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 5348.07 txn/s, latency: 6053.00 ms, (p50: 6500 ms, p70: 6600, p90: 7800 ms, p99: 8300 ms), latency samples: 187500
3. Upgrading rest of first batch to new version: 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7429.42 txn/s, latency: 3889.74 ms, (p50: 4300 ms, p70: 4600, p90: 4800 ms, p99: 4800 ms), latency samples: 141000
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6554.56 txn/s, latency: 4587.75 ms, (p50: 4700 ms, p70: 4800, p90: 5000 ms, p99: 6500 ms), latency samples: 247020
4. upgrading second batch to new version: 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 5342.35 txn/s, latency: 3850.29 ms, (p50: 2700 ms, p70: 4800, p90: 8500 ms, p99: 13300 ms), latency samples: 130140
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11400.81 txn/s, latency: 2723.94 ms, (p50: 2700 ms, p70: 2800, p90: 3000 ms, p99: 3300 ms), latency samples: 373460
5. check swarm health
Compatibility test for 7eeba4cd15892717741a614add1afde004c7855f ==> 651f2de0a2f6767f95c1d6cde719ddc6a61fc6b7 passed
Test Ok

@rahxephon89 rahxephon89 merged commit b481f94 into main Oct 15, 2024
89 of 95 checks passed
@rahxephon89 rahxephon89 deleted the teng/fix-14762 branch October 15, 2024 23:45
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.

[Bug][Compiler-v2][Prover] CALL_BORROWED_MUTABLE_REFERENCE_ERROR is raised when there is an inline spec
4 participants