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

Migrate to parking_lot #374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

StefanBossbaly
Copy link
Contributor

What

Migrate from std implementations of Mutex and RwLock to the parking_lot implementations. They are faster and more flexible than the those found in the Rust standard library. The parking_lot implementation also can not be poisoned, meaning that any Mutex / RwLock operations do not return a Result<> making the call to unwrap() no longer needed.

This change does make parking_lot crate a part of the ripple_sdk. It is up to the Ripple team to determine if that is an acceptable change.

Why

  • Faster processing of Mutex and RwLock operations
  • 186 unwrap()s removed.
  • Smaller code size. Each time unwrap() is called a conditional must be inserted with a panic for the Err variant.

How

By swapping std::sync::Mutex with parking_lot::Mutex and std::sync::RwLock with parking_lot::RwLock.

Test

TBD

Checklist

  • I have self-reviewed this PR
  • I have added tests that prove the feature works or the fix is effective

Sorry, something went wrong.

@brendanobra brendanobra added the needs-onboarding Changes which need to be onboarded for testing label Jan 5, 2024
@pahearn73 pahearn73 self-requested a review February 29, 2024 19:01
@pahearn73
Copy link
Contributor

I would be interested in migrating to parking lot, but before we do we should get some benchmark numbers against ripple's current implementation. Mostly interested in the code size delta.

Copy link
Contributor

@pahearn73 pahearn73 left a comment

Choose a reason for hiding this comment

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

I would be interested in migrating to parking lot, but before we do we should get some benchmark numbers against ripple's current implementation. Mostly interested in the code size delta.

@StefanBossbaly StefanBossbaly force-pushed the migrate_to_parking_lot branch 2 times, most recently from ea41a56 to 6876b96 Compare April 7, 2024 17:36
@StefanBossbaly StefanBossbaly force-pushed the migrate_to_parking_lot branch from 6876b96 to e2b3737 Compare April 19, 2024 03:37
Migrate from std implementations of Mutex and RwLock to the parking_lot
implementations. They are faster and more flexible than the those found
in the Rust standard library.
@StefanBossbaly StefanBossbaly force-pushed the migrate_to_parking_lot branch from 17d82d7 to 550b177 Compare April 19, 2024 03:55
@StefanBossbaly
Copy link
Contributor Author

StefanBossbaly commented Apr 19, 2024

@pahearn73 @brendanobra I rebased the commit on top of the current commit of the repo. Below is the results of the code size. Both where compiled with cargo build --release. With the main ripple executable you save 63432 bytes from switching from std::sync::{Mutex, RwLock} to parking_lot::{Mutex, RwLock}. Below is the full results.

Without parking_lot

$ pwd
/home/stefan/projects/Ripple/target/release
$ git rev-parse HEAD 
7ff4ae48243f28ffa4a1d06e06f7648011947aa9
$ ls -la
total 145800
drwxr-xr-x. 1 stefan stefan      826 Apr 19 00:00 ./
drwxr-xr-x. 1 stefan stefan       70 Apr 19 00:00 ../
drwxr-xr-x. 1 stefan stefan     3088 Apr 18 23:58 build/
-rw-r--r--. 1 stefan stefan        0 Apr 18 23:58 .cargo-lock
drwxr-xr-x. 1 stefan stefan    44870 Apr 19 00:00 deps/
drwxr-xr-x. 1 stefan stefan        0 Apr 18 23:58 examples/
drwxr-xr-x. 1 stefan stefan    15714 Apr 18 23:58 .fingerprint/
drwxr-xr-x. 1 stefan stefan        0 Apr 18 23:58 incremental/
-rw-r--r--. 1 stefan stefan     7664 Apr 19 00:00 libdistributor_general.d
-rwxr-xr-x. 2 stefan stefan 11477512 Apr 18 23:59 libdistributor_general.so*
-rw-r--r--. 1 stefan stefan     7294 Apr 19 00:00 liblauncher.d
-rwxr-xr-x. 2 stefan stefan 11060592 Apr 18 23:59 liblauncher.so*
-rw-r--r--. 1 stefan stefan     7440 Apr 19 00:00 libmock_device.d
-rwxr-xr-x. 2 stefan stefan 11685752 Apr 18 23:59 libmock_device.so*
-rw-r--r--. 1 stefan stefan      129 Apr 19 00:00 libopenrpc_validator.d
-rw-r--r--. 2 stefan stefan   894174 Apr 18 23:58 libopenrpc_validator.rlib
-rw-r--r--. 1 stefan stefan     6679 Apr 19 00:00 libripple_sdk.d
-rw-r--r--. 2 stefan stefan 39590608 Apr 18 23:59 libripple_sdk.rlib
-rw-r--r--. 1 stefan stefan     6971 Apr 19 00:00 libripple_tdk.d
-rw-r--r--. 2 stefan stefan    75502 Apr 18 23:58 libripple_tdk.rlib
-rw-r--r--. 1 stefan stefan     7037 Apr 19 00:00 librpc_extn.d
-rwxr-xr-x. 2 stefan stefan  8940328 Apr 18 23:59 librpc_extn.so*
-rw-r--r--. 1 stefan stefan     9282 Apr 19 00:00 libthunder.d
-rw-r--r--. 1 stefan stefan     9095 Apr 19 00:00 libthunder_ripple_sdk.d
-rw-r--r--. 2 stefan stefan 11617154 Apr 18 23:59 libthunder_ripple_sdk.rlib
-rwxr-xr-x. 2 stefan stefan 20135672 Apr 18 23:59 libthunder.so*
-rw-r--r--. 1 stefan stefan     6864 Apr 19 00:00 libtm_extn.d
-rwxr-xr-x. 2 stefan stefan 10620992 Apr 18 23:59 libtm_extn.so*
-rwxr-xr-x. 2 stefan stefan 23074560 Apr 19 00:00 ripple*
-rw-r--r--. 1 stefan stefan    13891 Apr 19 00:00 ripple.d

With parking_lot

 $ pwd 
/home/stefan/projects/Ripple/target/release
$ git rev-parse HEAD
550b17775e6d0fc8bf510e9bcf432e01d2e96402
$ ls -la
total 145604
drwxr-xr-x. 1 stefan stefan      826 Apr 18 23:55 ./
drwxr-xr-x. 1 stefan stefan      136 Apr 18 23:50 ../
drwxr-xr-x. 1 stefan stefan     3088 Apr 18 23:33 build/
-rw-r--r--. 1 stefan stefan        0 Apr 18 23:33 .cargo-lock
drwxr-xr-x. 1 stefan stefan    44870 Apr 18 23:55 deps/
drwxr-xr-x. 1 stefan stefan        0 Apr 18 23:33 examples/
drwxr-xr-x. 1 stefan stefan    15714 Apr 18 23:33 .fingerprint/
drwxr-xr-x. 1 stefan stefan        0 Apr 18 23:33 incremental/
-rw-r--r--. 1 stefan stefan     7664 Apr 18 23:36 libdistributor_general.d
-rwxr-xr-x. 2 stefan stefan 11477384 Apr 18 23:54 libdistributor_general.so*
-rw-r--r--. 1 stefan stefan     7294 Apr 18 23:36 liblauncher.d
-rwxr-xr-x. 2 stefan stefan 11057792 Apr 18 23:54 liblauncher.so*
-rw-r--r--. 1 stefan stefan     7440 Apr 18 23:36 libmock_device.d
-rwxr-xr-x. 2 stefan stefan 11687688 Apr 18 23:54 libmock_device.so*
-rw-r--r--. 1 stefan stefan      129 Apr 18 23:36 libopenrpc_validator.d
-rw-r--r--. 2 stefan stefan   894174 Apr 18 23:34 libopenrpc_validator.rlib
-rw-r--r--. 1 stefan stefan     6679 Apr 18 23:36 libripple_sdk.d
-rw-r--r--. 2 stefan stefan 39509548 Apr 18 23:53 libripple_sdk.rlib
-rw-r--r--. 1 stefan stefan     6971 Apr 18 23:36 libripple_tdk.d
-rw-r--r--. 2 stefan stefan    75502 Apr 18 23:53 libripple_tdk.rlib
-rw-r--r--. 1 stefan stefan     7037 Apr 18 23:36 librpc_extn.d
-rwxr-xr-x. 2 stefan stefan  8944624 Apr 18 23:53 librpc_extn.so*
-rw-r--r--. 1 stefan stefan     9282 Apr 18 23:36 libthunder.d
-rw-r--r--. 1 stefan stefan     9095 Apr 18 23:36 libthunder_ripple_sdk.d
-rw-r--r--. 2 stefan stefan 11571900 Apr 18 23:53 libthunder_ripple_sdk.rlib
-rwxr-xr-x. 2 stefan stefan 20108520 Apr 18 23:54 libthunder.so*
-rw-r--r--. 1 stefan stefan     6864 Apr 18 23:36 libtm_extn.d
-rwxr-xr-x. 2 stefan stefan 10636712 Apr 18 23:54 libtm_extn.so*
-rwxr-xr-x. 2 stefan stefan 23011128 Apr 18 23:55 ripple*
-rw-r--r--. 1 stefan stefan    13891 Apr 18 23:36 ripple.d

As far as a comparison is considered, you will have todo that testing as I don't have access to a device. If you did a perf run I would look for a decrease in time for futex syscalls, specially the FUTEX_WAIT calls.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-onboarding Changes which need to be onboarded for testing
Projects
Status: Review
Development

Successfully merging this pull request may close these issues.

None yet

4 participants