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.

Sign up for GitHub

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

enable optimizer in tests and procmacros #5155

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
+13 −8
Draft

enable optimizer in tests and procmacros #5155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
112b8f3
enable optimizer in tests and procmacros
alexpyattaev Mar 5, 2025
9531071
remove UB-checking assert from accountsdb test
Mar 5, 2025
3a892cc
hotfix for gossip test flaking
Mar 7, 2025
40a2be1
eliminate dead code
Mar 7, 2025
e0ba967
stragglers
Mar 7, 2025
3627c25
also optimize deps to O2 level
Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,18 @@ lto = false # Preserve the 'thin local LTO' for this build.
split-debuginfo = "unpacked"
lto = "thin"

# Enable basic optimizations for dev builds and unittests to trigger const propagation and inlining
[profile.dev]
opt-level = 1

# Enable optimizations for procmacros for faster codegen
[profile.dev.build-override]
opt-level = 1

# Enable heavier optimizations in deps (we do not rebuild them every time in dev cycle)
[profile.dev.package."*"]
opt-level = 2

[workspace]
members = [
"account-decoder",
Expand Down
8 changes: 0 additions & 8 deletions accounts-db/src/append_vec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1277,13 +1277,6 @@ pub mod tests {
}
}

fn get_executable_byte(&self) -> u8 {
Copy link
Author

Choose a reason for hiding this comment

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

this function was only used in a test

Copy link

Choose a reason for hiding this comment

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

This change seems unrelated to the unit tests speed up.
Why not move it into a separate PR?

Copy link
Author

Choose a reason for hiding this comment

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

The only reason this change is happening is to make optimizations possible for unittests. For more substantial changes separate PRs have been made by teams handling respective pieces of the code. I'll be making some as well.

Copy link

Choose a reason for hiding this comment

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

If this check is not really useful (as is claimed in the discussion) it can be done in a separate PR, as it has value by itself.
Making things simpler and avoiding UB, IIUC.
And merging smaller PRs is generally easier.

For example, should you end up introducing a separate compilation profile for the CI, this PR will increase in size and will take some time to land.

But this particular change might go in pretty quickly on its own.

It is up to you, of course. I was just speaking from personal experience :)

let executable_bool: bool = self.executable();
// UNSAFE: Force to interpret mmap-backed bool as u8 to really read the actual memory content
let executable_byte: u8 = unsafe { std::mem::transmute::<bool, u8>(executable_bool) };
executable_byte
}

fn set_executable_as_byte(&self, new_executable_byte: u8) {
// UNSAFE: Force to interpret mmap-backed &bool as &u8 to write some crafted value;
unsafe {
Expand Down Expand Up @@ -1830,7 +1823,6 @@ pub mod tests {
{
let executable_bool: bool = account.executable();
assert!(!executable_bool);
Copy link
Author

Choose a reason for hiding this comment

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

Removed this assert as per advice from @roryharr

Copy link

@roryharr roryharr Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

More Context: This assert was added a long time ago by ryoqun just to monitor some undefined behaviour.

The executable bit is a boolean which uses a byte of storage. The only valid values for the byte are zero and one. This is verified during account load by the function sanitize_executable.

This test corrupts the byte, and verifies that sanitize executable fails, which it does. Afterwards it has various asserts to see how it behaves in the event the sanitize check was not present. As per ryoquns comments these are purely for his interest.

When compiling with optimizations on, the compiler behaviour changes when accessing the byte by value.
u8 = unsafe { std::mem::transmute::<bool, u8>(executable_bool) }; returns the raw byte, rather than the underlying data converted to a bool, and then converted to a u8.

#1485 (comment)

Copy link

Choose a reason for hiding this comment

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

@ryoqun In case you are still interested

assert_eq!(account.get_executable_byte(), 0); // Wow, not crafted_executable!
}
})
.unwrap();
Expand Down
1 change: 1 addition & 0 deletions gossip/src/cluster_info.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3752,6 +3752,7 @@ mod tests {
let mut node = cluster_info.my_contact_info.write().unwrap();
node.set_shred_version(42);
}
std::thread::sleep(Duration::from_millis(1));
cluster_info.refresh_my_gossip_contact_info();
cluster_info.flush_push_queue();
// Should now include both epoch slots.
Expand Down
Loading