Macchina-CLI / libmacchina

A library providing access to all sorts of system information.
https://crates.io/crates/libmacchina
MIT License
68 stars 20 forks source link

Added __system_property_get calls to replace getprop calls. #30

Closed uttarayan21 closed 3 years ago

uttarayan21 commented 3 years ago

Performance increase around ~70ms

hyperfine

Old ~Performance increase around 50ms~ ~screenshot~

uttarayan21 commented 3 years ago

The call to pm list packages is what takes a lot of time (~50ms). ~The call to dpkg -l takes around 20ms~. Done

uttarayan21 commented 3 years ago

The CPU info of android could be better. maccina

grtcdr commented 3 years ago

Great work!

70ms improvement is truly a wonderful improvement, I can't wait to see how low we can get it.

I do think CPU information on android isn't as descriptive as I'd like it to be, but there's a slight annoyance, if we append the contents of hardware to the existing string, we're going to potentially have a string that is just too long, for no reason.

We might have to do some replacing.

Also I'm thinking of making relative shell path the default setting, what do you think?

grtcdr commented 3 years ago

The call to pm list packages is what takes a lot of time (~50ms).

I assume there's a way to get this information without querying pm, and it seems like running mosts command on Android takes longer than it should..

uttarayan21 commented 3 years ago

Great work!

70ms improvement is truly a wonderful improvement, I can't wait to see how low we can get it.

I do think CPU information on android isn't as descriptive as I'd like it to be, but there's a slight annoyance, if we append the contents of hardware to the existing string, we're going to potentially have a string that is just too long, for no reason.

We might have to do some replacing.

Also I'm thinking of making relative shell path the default setting, what do you think?

Thanks,

Yeah I was thinking of replacing ARMv8 Processor rev 4 (v8l) (8) with something like Qualcomm Technologies, Inc SDM636 (ARMv8) (8).

Yeah the shell path on termux is unnecessarily long. I haven't tried the relative setting yet so I can't say but something like fish (3.2.1) could also work.

As for a pm list packages replacement I don't know of any system calls and there is no way to read the data directly filesystem since /data partition has /data/app folder which has all user installed apps and it doesn't have --X permission for others. Also most stuff has SELinux permissions so It can't really be manually read.

grtcdr commented 3 years ago

Yeah I was thinking of replacing ARMv8 Processor rev 4 (v8l) (8) with something like Qualcomm Technologies, Inc SDM636 (ARMv8) (8).

Brilliant, I'd love to see this too.

Yeah the shell path on termux is unnecessarily long. I haven't tried the relative setting yet so I can't say but something like fish (3.2.1) could also work.

All is good until you mentioned 3.2.1, which is sort of something we have struggled to obtain since day 1, we can't access say, $ZSH_VERSION or $BASH_VERSION, trying to get these values results in empty strings.

The reason behind this is stated somewhere in the Rust docs, but it's something along the lines of, "the program's argument are passed literally to the program, not to the shell" which also means we can't get these shell-specific variables.

Also most stuff has SELinux permissions so It can't really be manually read.

Sh*t

grtcdr commented 3 years ago

Would you like to continue improving this or shall I merge?

uttarayan21 commented 3 years ago

I was thinking more like fish -v Performance impact should be minimal fish => fish -v bash => bash -c "printf %s \"\$BASH_VERSION\""

❯ hyperfine 'fish -v'
Benchmark #1: fish -v
  Time (mean ± σ):       3.2 ms ±   0.2 ms    [User: 2.5 ms, System: 1.4 ms]
  Range (min … max):     2.7 ms …   4.2 ms    485 runs

  Warning: Command took less than 5 ms to complete. Results might be inaccurate.
❯ hyperfine 'bash -c "printf %s \"\$BASH_VERSION\""'
Benchmark #1: bash -c "printf %s \"\$BASH_VERSION\""
  Time (mean ± σ):       1.6 ms ±   0.4 ms    [User: 1.5 ms, System: 1.1 ms]
  Range (min … max):     1.0 ms …   3.0 ms    595 runs

  Warning: Command took less than 5 ms to complete. Results might be inaccurate.
uttarayan21 commented 3 years ago

Would you like to continue improving this or shall I merge?

I'll have to make the /proc/cpuinfo change. Then I'll ping you to merge.

grtcdr commented 3 years ago

fish => fish -v

3ms is kinda significant unfortunately, and the problem with this approach is we have to parse the -v output, which is different across shells, bash for example likes to print way too many things when invoked with -v.

uttarayan21 commented 3 years ago

So I was going to do this but it seems that but it seems that

u0_a72 in localhost in ~ 
❯ cat /proc/cpuinfo | grep -i hardware | head -1
Hardware    : Qualcomm Technologies, Inc SDM636

u0_a72 in localhost in ~ 
❯ cat /proc/cpuinfo | grep -i model | head -1
model name  : ARMv8 Processor rev 4 (v8l)

So we might have to parse model with space seperator and display accordingly 😐 . So just printing Qualcomm Technologies, Inc SDM636 (8) might be better.

uttarayan21 commented 3 years ago

@grtcdr I have the commit ready should I also bump the version ?

grtcdr commented 3 years ago

My phone doesn't have "Hardware" field listed, so maybe we should prioritize Hardware when we find it, and if we don't then we print the "Processor" field.

Screenshot_20210422_194753_com.termux.jpg

uttarayan21 commented 3 years ago

Oh nice catch ! Mine is running a custom rom so that might be the reason why I have hardware. Anyway I'll prioritize hardware and then try cpu model.

uttarayan21 commented 3 years ago

Added fallback to cpu model I also bumped the version to 0.2.9

grtcdr commented 3 years ago

Added fallback to cpu model I also bumped the version to 0.2.9

We might need another fallback, that is "Processor" with a capital P, as I don't have "cpu model" in there.

Screenshot_20210422_200404.jpg

uttarayan21 commented 3 years ago

Haha I'll add a fallback to that too.

grtcdr commented 3 years ago

There's no reason to test if var.is_none() because they're initially none, we need to use else if rather than if, too.

uttarayan21 commented 3 years ago

Oh yeah I just copy pasted that since cpu model might appear multiple times so I'd like to avoid initializing them multiple times . I'll remove the is_none for hardware and Processor since they appear once and add else if

uttarayan21 commented 3 years ago

Also while testing I just realized that the android ascii art legs are asymmetric and its really bugging me lol. android

grtcdr commented 3 years ago

Feel free to fix it :)

uttarayan21 commented 3 years ago

@grtcdr You can merge it if you feel it's ready.

grtcdr commented 3 years ago

I don't see a reason to use header files, __system_get_prop is already in libc.

uttarayan21 commented 3 years ago

If I try to use __system_property_get from libc it doesn't compile.

libmacchina on  main [!] is 📦 v0.2.9 via 🦀 v1.53.0-nightly took 36s 
❯ cross build --target=aarch64-linux-android --release
   Compiling libmacchina v0.2.9 (/project)
error[E0432]: unresolved import `libc::__system_property_get`
 --> src/android/system_properties.rs:5:5
  |
5 | use libc::__system_property_get;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ no `__system_property_get` in the root

error: aborting due to previous error