486c / rosu-memory

Lightweight, cross-platform memory reader for osu! but in rust
24 stars 7 forks source link

Signature cleanup #3

Closed MaxOhn closed 11 months ago

MaxOhn commented 11 months ago

Refactored a few Signature things and minorly improved performance.

486c commented 11 months ago

Hello, huge thanks for your PR's. Just find out another edge case when formatting Signature into string. For example lets take string "07" convert it into Signature struct and then back to string and we gonna get "7" instead of "07" :smiley:

Consider doing this:

-SignatureByte::Byte(byte) => write!(f, "{byte:X}"),
+SignatureByte::Byte(byte) => write!(f, "{byte:02X}"),

plus i guess we should add tests for this particular edge case:

fn test_formatting() {
+     let expected = "07";
+     let s = Signature::from_str(expected).unwrap();
+     assert_eq!(s.bytes.len(), 1);
+     assert_eq!(expected, s.to_string());
}

If you don't have time then i'm gonna merge this and change it by myself, i'm fine with both options.

MaxOhn commented 11 months ago

Hello, huge thanks for your PR's. Just find out another edge case when formatting Signature into string. For example lets take string "07" convert it into Signature struct and then back to string and we gonna get "7" instead of "07" 😃

Oh good catch, will fix ASAP 👍

486c commented 11 months ago
Also was really curious about this new, beautiful, lovely, cute, idiomatic function find_signature(...) so did i quick benchmark. Here's results: runs (500 samples each) sig_find_old (avg) sig_find_new (avg)
1 114.91 ms 105.61 ms
2 114.89 ms 106.42 ms
3 113.62 ms 104.73 ms
4 114.61 ms 105.69 ms

Benchmark algorithm:

  1. Take 100 MB buffer of random numbers
  2. Take last 12 bytes as signature
  3. Scan buffer

so... with this PR we also gaining a bit of performance on initial addresses scanning! :partying_face: (too lazy to compare with gosu and tosu but i guess it's already faster anyway)

If someone reading this looking how to improve performance even further i recommend looking into process_vm_readv(2) syscall, since it can take multiple addresses and perform 'batch' reading (not sure about this one because i'm too lazy to look into internals, correct me if i'm wrong)

486c commented 11 months ago

Huge thanks for PR's <3