RFC: A feature for every readout type.

[grtcdr]

I think libmacchina can greatly benefit if we were to lock each readout type
behind its respective feature. e.g.:

• "package" feature for PackageReadout
• "general" feature for GeneralReadout

I believe this would entice developers that are not willing to ship their
programs with a library that brings a handful of dependencies, and I am one of
them (sort of). As I've been busy working on citron, which uses libmacchina as its
backend, I've had to deal with dependencies that just don't make any sense for
a program like citron.

A change like this could possibly result change like this will result in multiple API breakages, since if we were to truly implement this, it would be in our best interest to modularize libmacchina as much as possible (to avoid breaking the API more and more). An example of this is to create more readout types that have their own set of dependencies, and lower the method count of GeneralReadout type. As it currently has a little too much to offer.

Here's a video of #111 in action:
https://user-images.githubusercontent.com/35816711/147727323-958fb622-cd83-4709-a795-5d5151d952a1.mp4

What are your thoughts?

[grtcdr]

@uttarayan21 mentioned in our chat in the (lib)macchina matrix room, that we continue to package libmacchina with all features enabled by default (if we were to implement this proposed change).

In other words, application developers that are interested in libmacchina can either use all the readouts we offer (by default), or cherry-pick the readouts they want to use.

I totally agree with this, this is our best bet to keep things simple on the outside.

[grtcdr]

Here are some of the new readouts I propose:

pub trait GraphicalReadout {
    /// This function should return the name of the used desktop environment.
    ///
    /// _e.g._ `Plasma`
    fn desktop_environment(&self) -> Result<String, ReadoutError>;

    /// This function should return the type of session that's in use.
    ///
    /// _e.g._ `Wayland`
    fn session(&self) -> Result<String, ReadoutError>;

    /// This function should return the name of the used window manager.
    ///
    /// _e.g._ `KWin`
    fn window_manager(&self) -> Result<String, ReadoutError>;

    /// This function should return the name of the used terminal emulator.
    ///
    /// _e.g._ `kitty`
    fn terminal(&self) -> Result<String, ReadoutError>;
}

pub trait ProcessorReadout {
    /// This function should return the model name of the CPU \
    ///
    /// _e.g._ `Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz`
    fn cpu_model_name(&self) -> Result<String, ReadoutError>;

    /// This function should return the average CPU usage over the last minute.
    fn cpu_usage(&self) -> Result<usize, ReadoutError>;

    /// This function should return the number of physical cores of the host's processor.
    fn cpu_physical_cores(&self) -> Result<usize, ReadoutError>;

    /// This function should return the number of logical cores of the host's processor.
    fn cpu_cores(&self) -> Result<usize, ReadoutError>;
}

These have obviously been moved from GeneralReadout.

[grtcdr]

The linked pull request was written purely as a demonstration. It's only working on Linux right now, since it makes some pretty bold breaking changes to the API (that would take a couple days to port over to all other platforms). It does however deliver on the one thing that it promises: modularity.

[FantasyTeddy]

I really like the idea of allowing users of the library choose which parts (dependencies) are needed for their application.

However there is one concern that I have with introducing even more readouts: How can we share state between multiple different readouts?
Let me try to explain:

• Example 1: The Linux implementation of multiple readouts stores a sysinfo struct and even though this is not a particularly expensive operation, we could imagine that if we wanted to speed things up, we would maybe also perform the call to sysinfo() only once. Therefore it would be desirable to be able to store such state information at one central place instead of once per readout.
• Example 2: As mentioned in [Feature] Windows related improvements #112 the WMI information on Windows will probably be used even more when implementing more readouts (like resolution or backlight). For querying the WMI information a connection needs to be opened which is quite expensive. It would therefore also be beneficial if the connection could be opened once and reused for each separate query.

This concern is only slightly related to this proposed change, but I wanted to share my thoughts on this and maybe you have some ideas how we could solve this (if we need to).

[grtcdr]

I think it's important that you brought this up, I have always wondered how many milliseconds we can shave off just by saving a state instead of reinstantiating the necessary structs.

I haven't experimented with a solution, since the Linux implementation is so fast, any gained time is barely noticeable, but I always like seeing smaller numbers. Also, I'm unhappy with the Windows performance, so we definitely need to do something about this.

I can't say I know how to approach this correctly and idiomatically, but I will experiment with some ideas. We should have a separate issue for this, because we need to discuss how to translate this idea into code properly.

Before we continue, though, I want to see what the others think of this RFC, so we can all pour our best work in #111 instead of main.

[grtcdr]

I believe it won't be possible to share objects between traits without compromising on the "one trait multiple implementations" setup that we've got.

I think that if we truly desire the utmost performance, most if not all platforms will have to have their own implementations without a trait to adhere to, very similar to how libc works. We'll basically be writing our own (untested) "rusty-libc" which as far as I know is being worked on, by some very smart folks.

I say this because I've just spent a couple hours trying to figure out how to make it work, and it's just not possible, for example:

LinuxProcessorReadout::cpu_usage() makes use of sysinfo, while the hypothetical MacOSProcessorReadout::cpu_usage() does not. If I make it so cpu_usage() takes a reference to a pointer of type sysinfo as one of its parameters, but that just won't work for macOS because it doesn't make use of sysinfo.

EDIT: I'm not giving up though, at least not yet, I'll continue looking for alternative ways to achieve this. Maybe this is a good case to hand this over to lazy_static

EDIT 2: Or not, the maintainers did suggest just using Rust's own "unsafe mutable static feature", which I will try right away.

[FantasyTeddy]

One way to share some state would be to go the other way: reduce the number of different readouts to only one and every implementation can define their own shared state.
However I don't think that this is necessarily a good solution, since I like the way that the current readouts are split up to make it clear what information they offer.
I also don't know how (and if) this would work with the proposed feature flags.

[grtcdr]

One way to share some state would be to go the other way: reduce the number of different readouts to only one and every implementation can define their own shared state.

Well, there wouldn't be much of a point to this RFC if we were to do this, unfortunately. And like you said, we would lose out on context.

[grtcdr]

I also don't know how (and if) this would work with the proposed feature flags.

I almost managed to get this to work, until I hit a roadblock I'm not sure I can recover from. Here's what I've been staring at for the last 40 minutes:

error[E0015]: calls in constants are limited to constant functions, tuple structs and tuple variants
  --> src/linux/mod.rs:49:46
   |
49 |         const return_value: c_int = unsafe { sysinfo(sys_ptr) };
   |                                              ^^^^^^^^^^^^^^^^

This is due to the fact that libc::sysinfo is not a constant function.

A possible remedy would be these lines that I uncommented:

pub struct LinuxSharedObjects {
    sysinfo: *const sysinfo,
}

impl LinuxSharedObjects {
    const fn new() -> Self {
        const sys: sysinfo = sysinfo::new();
        const sys_ptr: *const sysinfo = &sys;
        // const return_value: c_int = unsafe { sysinfo(sys_ptr) };
        // if return_value != -1 {
        //     LinuxSharedObjects { sysinfo: sys_ptr }
        // } else {
        //     LinuxSharedObjects {
        //         sysinfo: std::ptr::null(),
        //     }
        // }
        LinuxSharedObjects { sysinfo: sys_ptr }
    }
}

The only gain here is that sysinfo isn't initialized multiple times, but I can't call const return_value: c_int = unsafe { sysinfo(sys_ptr) }. (It's not obvious but this function is part of the ffi module.

So ffi::sysinfo() will still have to be called more than once...

[grtcdr]

I'll try and get some benchmarks, but I don't want to get your hopes up, the only benefit of this is less allocations (which is still good, to be honest). libmacchina makes way too many of them, just take a look at valgrind macchina.

[FantasyTeddy]

Well, there wouldn't be much of a point to this RFC if we were to do this, unfortunately. And like you said, we would lose out on context.

I'm not very familiar how features work in Rust, but I imagined that you could also exclude/include single functions instead of entire structs. If this is not how this works then this RFC would indeed be incompatible with my suggestion.

I'll try and get some benchmarks, but I don't want to get your hopes up, the only benefit of this is less allocations (which is still good, to be honest). libmacchina makes way too many of them, just take a look at valgrind macchina.

Even though it would certainly be better to do the allocation only once, I agree that there is not much performance to be gained here. I think the second example I mentioned before (opening a WMI connection on Windows) would be a better argument where performance matters, since this is quite an expensive operation.

[grtcdr]

Even though it would certainly be better to do the allocation only once, I agree that there is not much performance to be gained here. I think the second example I mentioned before (opening a WMI connection on Windows) would be a better argument where performance matters, since this is quite an expensive operation.

The only possible way to achieve this as far as I can tell (I've been trying to piece it all together since you first suggested this idea) is to drop our traits module and have every platform be its own thing (unless we can figure out how to share a generic handle, that can be used along with the usual traits). This way, you could start the connection in the constructor, and pass a reference to the connection to all functions that need it.

A change like this would warrant another RFC, because it's just as big a change as this one.

[FantasyTeddy]

Exactly.
I guess you got my point and we can leave it at that for the moment, because like you just mentioned (and already mentioned before) this would probably require an entirely different discussion and is kind of off-topic here.

[grtcdr]

I've already reverted the changes I previously pushed, the complexity (of my half-solution) that it introduced is just not worth it, hopefully we can pick this up some other day.

[grtcdr]

I'm not very familiar how features work in Rust

It's my first time making use of features (that's a lie, it's my second, first feature was to only expose our commit hash if the hash feature is enabled)

so I'm not an expert by any means.

but I imagined that you could also exclude/include single functions instead of entire structs

That is totally possible, just about anything can be made a feature, even single lines of code.

[rashil2000]

I think the second example I mentioned before (opening a WMI connection on Windows) would be a better argument where performance matters, since this is quite an expensive operation.

As @FantasyTeddy already mentions, a lot readouts will require WMI queries. (In winfetch too we use it for around 10-12 readouts, but all that is done through a single connection - called CimSession shown here).

Will it be possible to lock readouts that require WMI behind one feature? Instead of having separate features for example for OSName, Backlight and Resolution, we can one feature for stuff fetched through WMI.

On a similar note, will it be possible to bunch-up all Linux readouts together that require the sysinfo dependency? I assume it's being used to provide a multitude of information categories.

Although this will turn the RFC from "A feature for every readout type" to "A feature for every dependency crate".
(aren't we looking to minimize dependencies after all?)

[grtcdr]

Your request contradicts what the RFC proposes.

If we were to implement shared objects (or a connection for Windows), it MUST be done at the root level of the platform e.g. windows/mod.rs or linux/mod.rs and the connection should only be opened when a feature that requires it is enabled.

This is currently not possible, not with the current state of macchina, nor with the RFC, as every platform implements each of its Readouts from the traits module, and you can't pass extra parameters to specific methods since you'll be going against the function's signature.

[rashil2000]

That makes sense, thanks for the info.