Skip to content

[Fix] Fix table info handling.#16264

Merged
grao1991 merged 1 commit intomainfrom
grao_fix_table_info
Apr 3, 2025
Merged

[Fix] Fix table info handling.#16264
grao1991 merged 1 commit intomainfrom
grao_fix_table_info

Conversation

@grao1991
Copy link
Contributor

@grao1991 grao1991 commented Apr 1, 2025

Description

For nested table case in the same batch, when the outer table is processed in ram, but before it got stored into db, the inner table handle may still be added into the pending_on (in a different thread, because the in memory result is not shared among threads). In this case we shouldn't blindly cleanup the pending_on map.

How Has This Been Tested?

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

@trunk-io
Copy link

trunk-io bot commented Apr 1, 2025

⏱️ 4h 16m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 1h 41m 🟩🟩🟩🟩
rust-smoke-tests 38m 🟩
execution-performance / test-target-determinator 19m 🟩🟩🟩🟩
test-target-determinator 19m 🟩🟩🟩🟩
forge-compat-test / forge 14m 🟩
check-dynamic-deps 11m 🟩🟩🟩🟩🟩
rust-cargo-deny 9m 🟩🟩🟩🟩🟩
rust-doc-tests 7m 🟩
rust-doc-tests 7m 🟩
rust-doc-tests 7m 🟩
rust-doc-tests 7m 🟩
fetch-last-released-docker-image-tag 6m 🟩🟩🟩🟩
cli-e2e-tests / run-cli-tests 5m 🟩
general-lints 2m 🟩🟩🟩🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩

🚨 2 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 26m 18m +43%
check-dynamic-deps 1m 3m -45%

settingsfeedbackdocs ⋅ learn more about trunk.io

@grao1991 grao1991 requested review from bowenyang007 and jillxuu April 1, 2025 22:27
@grao1991 grao1991 marked this pull request as ready for review April 1, 2025 22:29
Copy link
Contributor

@bowenyang007 bowenyang007 left a comment

Choose a reason for hiding this comment

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

is it tested? is there a repro to the bug? seems like it was pretty bad for a long time lol.

@grao1991
Copy link
Contributor Author

grao1991 commented Apr 1, 2025

is it tested? is there a repro to the bug? seems like it was pretty bad for a long time lol.

My backfill node crashed because of this bug. It's not that easy trigger in non-backfill case because the traffic is low and no multithread is involved.

@grao1991 grao1991 enabled auto-merge (squash) April 1, 2025 22:49
@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.

@grao1991 grao1991 force-pushed the grao_fix_table_info branch from ced1bfc to 88139bb Compare April 1, 2025 23:44
@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.

@grao1991 grao1991 force-pushed the grao_fix_table_info branch from 88139bb to 37ad29c Compare April 3, 2025 01:46
@grao1991 grao1991 force-pushed the grao_fix_table_info branch from 37ad29c to 24b3bd2 Compare April 3, 2025 02:00
@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
Copy link
Contributor

github-actions bot commented Apr 3, 2025

✅ Forge suite realistic_env_max_load success on 24b3bd284c47a5d50b529b9709c38709cc00b802

two traffics test: inner traffic : committed: 4185.99 txn/s, latency: 9509.35 ms, (p50: 9400 ms, p70: 9800, p90: 10300 ms, p99: 13600 ms), latency samples: 1591600
two traffics test : committed: 99.97 txn/s, latency: 2767.95 ms, (p50: 1400 ms, p70: 1500, p90: 6800 ms, p99: 14100 ms), latency samples: 1860
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 7.702, avg: 7.204", "ConsensusProposalToOrdered: max: 0.293, avg: 0.290", "ConsensusOrderedToCommit: max: 0.495, avg: 0.405", "ConsensusProposalToCommit: max: 0.786, avg: 0.695"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.08s no progress at version 15751 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.72s no progress at version 859083 (avg 0.72s) [limit 16].
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

✅ Forge suite framework_upgrade success on da4d9d451fa6e3cd5583d4a9fee4d609588afb30 ==> 24b3bd284c47a5d50b529b9709c38709cc00b802

Compatibility test results for da4d9d451fa6e3cd5583d4a9fee4d609588afb30 ==> 24b3bd284c47a5d50b529b9709c38709cc00b802 (PR)
Upgrade the nodes to version: 24b3bd284c47a5d50b529b9709c38709cc00b802
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1693.44 txn/s, submitted: 1698.10 txn/s, failed submission: 4.66 txn/s, expired: 4.66 txn/s, latency: 1761.73 ms, (p50: 1600 ms, p70: 1900, p90: 2400 ms, p99: 3600 ms), latency samples: 152560
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1736.98 txn/s, submitted: 1743.56 txn/s, failed submission: 6.58 txn/s, expired: 6.58 txn/s, latency: 1668.70 ms, (p50: 1500 ms, p70: 1800, p90: 2500 ms, p99: 3800 ms), latency samples: 158281
5. check swarm health
Compatibility test for da4d9d451fa6e3cd5583d4a9fee4d609588afb30 ==> 24b3bd284c47a5d50b529b9709c38709cc00b802 passed
Upgrade the remaining nodes to version: 24b3bd284c47a5d50b529b9709c38709cc00b802
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1707.74 txn/s, submitted: 1713.48 txn/s, failed submission: 5.74 txn/s, expired: 5.74 txn/s, latency: 1737.12 ms, (p50: 1500 ms, p70: 1900, p90: 2400 ms, p99: 3600 ms), latency samples: 154640
Test Ok

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2025

✅ Forge suite compat success on da4d9d451fa6e3cd5583d4a9fee4d609588afb30 ==> 24b3bd284c47a5d50b529b9709c38709cc00b802

Compatibility test results for da4d9d451fa6e3cd5583d4a9fee4d609588afb30 ==> 24b3bd284c47a5d50b529b9709c38709cc00b802 (PR)
1. Check liveness of validators at old version: da4d9d451fa6e3cd5583d4a9fee4d609588afb30
compatibility::simple-validator-upgrade::liveness-check : committed: 10863.96 txn/s, latency: 2906.83 ms, (p50: 3000 ms, p70: 3100, p90: 3500 ms, p99: 4000 ms), latency samples: 362920
2. Upgrading first Validator to new version: 24b3bd284c47a5d50b529b9709c38709cc00b802
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 2910.21 txn/s, latency: 9949.29 ms, (p50: 10800 ms, p70: 12500, p90: 12800 ms, p99: 12800 ms), latency samples: 62920
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 2936.70 txn/s, latency: 11321.53 ms, (p50: 12700 ms, p70: 13000, p90: 13100 ms, p99: 13400 ms), latency samples: 112960
3. Upgrading rest of first batch to new version: 24b3bd284c47a5d50b529b9709c38709cc00b802
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 2825.12 txn/s, latency: 10359.23 ms, (p50: 11200 ms, p70: 12900, p90: 13500 ms, p99: 13600 ms), latency samples: 62280
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 2861.69 txn/s, latency: 11664.56 ms, (p50: 13200 ms, p70: 13500, p90: 13700 ms, p99: 13900 ms), latency samples: 108300
4. upgrading second batch to new version: 24b3bd284c47a5d50b529b9709c38709cc00b802
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 5292.19 txn/s, latency: 5793.45 ms, (p50: 6500 ms, p70: 7000, p90: 7500 ms, p99: 7800 ms), latency samples: 103180
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5374.02 txn/s, latency: 6358.12 ms, (p50: 6800 ms, p70: 7200, p90: 7500 ms, p99: 7800 ms), latency samples: 185980
5. check swarm health
Compatibility test for da4d9d451fa6e3cd5583d4a9fee4d609588afb30 ==> 24b3bd284c47a5d50b529b9709c38709cc00b802 passed
Test Ok

@grao1991 grao1991 merged commit f88ff29 into main Apr 3, 2025
46 checks passed
@grao1991 grao1991 deleted the grao_fix_table_info branch April 3, 2025 22:27
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.

4 participants