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

LayeredMap: carries Value instead of Option<Value> #13980

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

msmouse
Copy link
Contributor

@msmouse msmouse commented Jul 11, 2024

Description

The interface used to understand None as deletions (None as tombstones) and when trying to get a key, the result is the same if the key is not found or the value is None. The behavior confused the call site, I'd rather let the call site deal with the tombstones "manually.

Type of Change

  • Refactoring

Which Components or Systems Does This Change Impact?

  • Other (specify) -- experimental

How Has This Been Tested?

unit test

Copy link

trunk-io bot commented Jul 11, 2024

⏱️ 4h 23m total CI duration on this PR
Job Cumulative Duration Recent Runs
test-fuzzers 2h 36m 🟩🟩🟩🟥
forge-e2e-test / forge 28m 🟥🟩
forge-compat-test / forge 13m 🟩
general-lints 8m 🟩🟩🟩🟩
test-target-determinator 8m 🟩
execution-performance / test-target-determinator 8m 🟩
rust-cargo-deny 7m 🟩🟩🟩🟩
rust-move-tests 6m 🟩
rust-move-tests 6m 🟩
rust-move-tests 6m 🟩
rust-move-tests 5m 🟩
check-dynamic-deps 4m 🟩🟩🟩🟩
check 3m 🟩
semgrep/ci 1m 🟩🟩🟩🟩
file_change_determinator 52s 🟩🟩🟩🟩
file_change_determinator 46s 🟩🟩🟩🟩
file_change_determinator 13s 🟩
permission-check 13s 🟩🟩🟩🟩
permission-check 11s 🟩🟩🟩🟩
permission-check 11s 🟩🟩🟩🟩
forge-framework-upgrade-test / forge 11s 🟩
permission-check 10s 🟩🟩🟩🟩
execution-performance / single-node-performance 9s 🟩
Backport PR 3s 🟥
permission-check 3s 🟩
permission-check 2s 🟩
determine-docker-build-metadata 2s 🟩

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

Job Duration vs 7d avg Delta
test-target-determinator 8m 5m +62%
execution-performance / test-target-determinator 8m 6m +47%
test-fuzzers 47m 37m +29%
execution-performance / single-node-performance 9s 11m -99%

settingsfeedbackdocs ⋅ learn more about trunk.io

@msmouse msmouse force-pushed the 0702-alden-layered-map-no-opt branch from 3ce8a47 to d53c20d Compare July 11, 2024 16:55
@msmouse msmouse requested review from areshand and grao1991 July 11, 2024 16:58
Basically, no longer supports "deletion" from LayeredMap.
One can express deletion by explicitly using an MapLayer<K, Option<V>> (tombstone).
@msmouse msmouse force-pushed the 0702-alden-layered-map-no-opt branch from d53c20d to 99a8a02 Compare July 11, 2024 17:53
@msmouse msmouse marked this pull request as ready for review July 11, 2024 17:53
@msmouse msmouse enabled auto-merge (squash) July 17, 2024 23:09

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 99a8a02be3842645da430278dabae84d08b2df18

Compatibility test results for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 99a8a02be3842645da430278dabae84d08b2df18 (PR)
1. Check liveness of validators at old version: 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5
compatibility::simple-validator-upgrade::liveness-check : committed: 8553.841286084911 txn/s, latency: 3781.98145239246 ms, (p50: 2700 ms, p90: 6300 ms, p99: 27500 ms), latency samples: 331040
2. Upgrading first Validator to new version: 99a8a02be3842645da430278dabae84d08b2df18
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5834.582774366921 txn/s, latency: 4696.095072669266 ms, (p50: 4600 ms, p90: 5900 ms, p99: 6200 ms), latency samples: 112840
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6396.741509240357 txn/s, latency: 4623.616221405228 ms, (p50: 4600 ms, p90: 5300 ms, p99: 6300 ms), latency samples: 244800
3. Upgrading rest of first batch to new version: 99a8a02be3842645da430278dabae84d08b2df18
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7130.070705190575 txn/s, latency: 3681.9347265221877 ms, (p50: 4100 ms, p90: 4400 ms, p99: 4600 ms), latency samples: 135660
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 7045.52102357186 txn/s, latency: 4641.385065691003 ms, (p50: 4700 ms, p90: 5400 ms, p99: 6200 ms), latency samples: 240520
4. upgrading second batch to new version: 99a8a02be3842645da430278dabae84d08b2df18
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 6295.036619089573 txn/s, latency: 3680.300147453083 ms, (p50: 2500 ms, p90: 9400 ms, p99: 11700 ms), latency samples: 149200
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 9412.927988952872 txn/s, latency: 3510.703137884873 ms, (p50: 3000 ms, p90: 7000 ms, p99: 10100 ms), latency samples: 373500
5. check swarm health
Compatibility test for 1c2ee7082d6eff8c811ee25d6f5a7d00860a75d5 ==> 99a8a02be3842645da430278dabae84d08b2df18 passed
Test Ok

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 99a8a02be3842645da430278dabae84d08b2df18

two traffics test: inner traffic : committed: 9211.238283034294 txn/s, latency: 4351.413358782985 ms, (p50: 3900 ms, p90: 5100 ms, p99: 11300 ms), latency samples: 3502340
two traffics test : committed: 100.07932547647337 txn/s, latency: 2570.1 ms, (p50: 2000 ms, p90: 3300 ms, p99: 9700 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.241, avg: 0.223", "QsPosToProposal: max: 1.668, avg: 1.468", "ConsensusProposalToOrdered: max: 0.324, avg: 0.293", "ConsensusOrderedToCommit: max: 0.404, avg: 0.391", "ConsensusProposalToCommit: max: 0.695, avg: 0.684"]
Max round gap was 1 [limit 4] at version 967695. Max no progress secs was 5.816677 [limit 15] at version 3801043.
Test Ok

@msmouse msmouse merged commit bb69950 into main Jul 18, 2024
86 of 90 checks passed
@msmouse msmouse deleted the 0702-alden-layered-map-no-opt branch July 18, 2024 01:12
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