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

Remove unsafe #7667

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Remove unsafe #7667

merged 2 commits into from
Apr 10, 2023

Conversation

gerben-stavenga
Copy link
Contributor

Description

Over the weekend I investigated some SIGSEGV I experienced and was looking at unsafe code in our code base as possible culprits. I encountered this which I initially thought must be wrong (Option<Box> is type punned as an usize). It turns out that absolute compiler magic makes Option<Box> the same size as usize, but I feel that kind of code is ridiculous.

The problem is that one cannot use [None, NB_BUCKETS] as Bucket == Option<Box> is non-copyable, however one can simply work around that by using cont INIT: Bucket = None, and use INIT as initialization value.

This seems cleaner

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

Very clean indeed, I don't suppose you have a test that would demonstrate how to get the sigsegv?

@gerben-stavenga
Copy link
Contributor Author

The sigsegv was caused by stack overflow, (I had raised the stack limit with ulimit -s but that didnt help but wolfgang pointed me to env RUST_MIN_STACK) while investigating the memory I stumbled upon this

@gerben-stavenga gerben-stavenga enabled auto-merge (squash) April 10, 2023 16:34
@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 land_blocking success on 61dcf49f14ce4c9312d0ff138a415db4fb333763

performance benchmark with full nodes : 5608 TPS, 7049 ms latency, 12000 ms p99 latency,(!) expired 520 out of 2395380 txns
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 61dcf49f14ce4c9312d0ff138a415db4fb333763

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 61dcf49f14ce4c9312d0ff138a415db4fb333763 (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7770 TPS, 4904 ms latency, 7400 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: 61dcf49f14ce4c9312d0ff138a415db4fb333763
compatibility::simple-validator-upgrade::single-validator-upgrade : 4810 TPS, 8355 ms latency, 11400 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: 61dcf49f14ce4c9312d0ff138a415db4fb333763
compatibility::simple-validator-upgrade::half-validator-upgrade : 4492 TPS, 9043 ms latency, 12200 ms p99 latency,no expired txns
4. upgrading second batch to new version: 61dcf49f14ce4c9312d0ff138a415db4fb333763
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6496 TPS, 5937 ms latency, 11400 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> 61dcf49f14ce4c9312d0ff138a415db4fb333763 passed
Test Ok

@github-actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 61dcf49f14ce4c9312d0ff138a415db4fb333763

Compatibility test results for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 61dcf49f14ce4c9312d0ff138a415db4fb333763 (PR)
Upgrade the nodes to version: 61dcf49f14ce4c9312d0ff138a415db4fb333763
framework_upgrade::framework-upgrade::full-framework-upgrade : 6447 TPS, 6219 ms latency, 8600 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for aptos-node-v1.3.0_3fc3d42b6cfe27460004f9a0326451bcda840a60 ==> 61dcf49f14ce4c9312d0ff138a415db4fb333763 passed
Test Ok

@gerben-stavenga gerben-stavenga merged commit 224192f into main Apr 10, 2023
@gerben-stavenga gerben-stavenga deleted the remove_unsafe branch April 10, 2023 17:12
WGB5445 pushed a commit to WGB5445/aptos-core that referenced this pull request Apr 14, 2023
* remove unsafe

* add comment
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