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

Fix benchmarks #243

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Fix benchmarks #243

wants to merge 4 commits into from

Conversation

JustinCappos
Copy link
Member

@JustinCappos JustinCappos commented May 11, 2024

Description

Partial fix for #242

This fixes the two example problems listed in #242. However, #241 is not addressed here (which is noted in the code along with instructions about how to change the code after fixing this bug).

Furthermore, a bug still exists leading to some memory corruption issues. (Notice the XXXXXXX in the output below is indicative that a string with the test name was overwritten by the construction of deststring.)

I would appreciate help to track down the memory corruption issue.

`Compare fs:write+read+lseek/TF03:Lind write+read+lseek/1
time: [1.0006 µs 1.0080 µs 1.0200 µs]
change: [+2.9263% +3.7612% +4.6903%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) high mild
7 (7.00%) high severe
Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/64: Warming Benchmarking XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX: Collecti Benchmarking XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX: Analyzin XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
time: [1.0222 µs 1.0229 µs 1.0236 µs]
change: [+3.1119% +4.1536% +5.9263%] (p = 0.00 < 0.05)
Change within noise threshold.
Found 10 outliers among 100 measurements (10.00%)
6 (6.00%) high mild
4 (4.00%) high severe
Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/1024: Warmin Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/1024: Collec Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/1024: Analyz Compare fs:write+read+lseek/TF03:Lind write+read+lseek/1024
time: [1.0780 µs 1.0958 µs 1.1209 µs]
change: [+6.9239% +21.318% +44.944%] (p = 0.01 < 0.05)
Change within noise threshold.
Found 21 outliers among 100 measurements (21.00%)
4 (4.00%) high mild
17 (17.00%) high severe
Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/65536: Warmi Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/65536: Colle Benchmarking Compare fs:write+read+lseek/TF03:Lind write+read+lseek/65536: Analy Compare fs:write+read+lseek/TF03:Lind write+read+lseek/65536
time: [5.4057 µs 5.4267 µs 5.4614 µs]
Found 12 outliers among 100 measurements (12.00%)
1 (1.00%) low mild
3 (3.00%) high mild
8 (8.00%) high severe
error: bench failed, to rerun pass '--bench fs_read_write_seek'

Caused by:
process didn't exit successfully: '/home/test/safeposix-rust/target/release/deps/fs_read_write_seek-adecf1c62a8ef04d --bench' (signal: 11, SIGSEGV: invalid memory reference)
`

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Run on docker on my mac using cargo bench

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@JustinCappos
Copy link
Member Author

This memory corruption issue happens even when I comment out all of the lind code and just use the Native part of the benchmark.

I don't fully understand this issue, but I'm quite suspicious of tests::size2cbuf from our code. This discussion also seems to indicate the problem may be with how vectors are being used.

@JustinCappos
Copy link
Member Author

JustinCappos commented May 11, 2024

Note, the problem also occurs when I replace the file I/O, etc. with std::ptr::copy_nonoverlapping(deststring, read_buffer, *buflen);

A minimal example that replicates this is:

    for buflen in [1, 64, 1024, 65536].iter() {
        let deststring = tests::str2cbuf(
            &String::from_utf8(vec![b'X'; *buflen]).expect("error building string"),
        );
        let read_buffer = tests::sizecbuf(*buflen).as_mut_ptr();
        group.bench_with_input(
            BenchmarkId::new("TF03:Native write+read+lseek", buflen),
            buflen,
            |b, buflen| {
                b.iter(|| unsafe {
                    std::ptr::copy_nonoverlapping(deststring, read_buffer, *buflen);
                })
            },
        );
    }

@JustinCappos
Copy link
Member Author

It still crashes when replacing:
let read_buffer = tests::sizecbuf(*buflen).as_mut_ptr();
with

let mut c: Vec<u8> = Vec::new();
let mut read_buffer: *mut u8 = c.as_mut_ptr();

@JustinCappos
Copy link
Member Author

Okay, this fixes it:

        let mut actual_read_buffer = [0u8;65536];
        let mut read_buffer: *mut u8 = actual_read_buffer.as_mut_ptr();

My understanding is that this is because the bytes are actually allocated here.

As I understand it, my previous attempt with Vec.new() doesn't work because Rust doesn't allocate memory for Vectors until it is needed.

However, I'm still confused why https://github.com/Lind-Project/safeposix-rust/blob/fix_benchmarks/src/tests/mod.rs#L84C1-L88C2 doesn't work. From what I can see, it seems like this should still be a valid part of memory to access. Is this falling out of scope and being garbage collected when it shouldn't?

(I also need to get far enough through the Rust Book so I understand why a Box may do this. To me, this just seems like a malloced area on the heap now...)

@JustinCappos
Copy link
Member Author

Here is a minimal test case which shows the problem:

pub fn sizecbuf<'a>(size: usize) -> Box<[u8]> {
    let v = vec![0u8; size];
    v.into_boxed_slice()
}

fn main() {
    
    // Iterate to trigger the bug...
    for buflen in [1, 16, 64, 256, 1024, 4096, 65536].iter() {
        let src_vec: *mut u8 = vec![b'X'; *buflen].as_mut_ptr();

        let dest_buffer = sizecbuf(*buflen).as_mut_ptr();

// Alternative that also has the problem
/*        let v = vec![0u8; *buflen];
        let mut new_v:Box<[u8]> = v.into_boxed_slice();
        let mut dest_buffer = new_v.as_mut_ptr();  */

// Alternative which avoids the issue
//       let mut actual_dest_buffer = [0u8;65536];
//        let dest_buffer: *mut u8 = actual_dest_buffer.as_mut_ptr();


        for _ in 0..10 {
            unsafe {
               std::ptr::copy_nonoverlapping(src_vec, dest_buffer, *buflen);
            }
        }

        // Having a println in here makes it trigger more reliably
        println!("hi");
    }

}

So, I know how to fix this, but I'd like to understand why this is an issue. I would have thought 'Box'ing would allocate memory on the heap and was a safe way to do this.

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.

1 participant