odilia-app / atspi

A fast, zbus-based, permissively licensed AT-SPI library written in pure Rust!
Apache License 2.0
32 stars 6 forks source link

A 'zero-copy' `EventBody<'a, T>` without custom deserializer? #160

Closed luukvanderduim closed 4 months ago

luukvanderduim commented 8 months ago

In atspi-common/src/events/mod.rs:~45

We find this EventBody<'a, T> definition that has puzzled me for a while.

It's field kind is a generic T. In D-Bus, this is a string type s.

The comment says we may become &str or State and there might be more. It has been here as long as I know.

What was the idea? I imagine something with custom deserialization?

It seems not to be in use at the moment, and has not been for over a year. Correct?

While I am in favor of exploring a 'zero-copy' (borrowed) EventBody<'_> we should try that in a separate tree and not keep dead code around in main.

If it is used by externally, but not internally, we should probably write that in the comments to avoid this question in the future.

Thanks in advance!

/// A borrowed body for events.
#[derive(Debug, Serialize, Deserialize)]
pub struct EventBody<'a, T> {
    /// A generic "kind" type, defined by AT-SPI:
    /// usually a `&'a str`, but can be another type like [`crate::state::State`].
    #[serde(rename = "type")]
    pub kind: T,
    /// Generic first detail defined by AT-SPI.
    pub detail1: i32,
    /// Generic second detail defined by AT-SPI.
    pub detail2: i32,
    /// Generic `any_data` field defined in AT-SPI.
    /// Can contain any type.
    #[serde(borrow)]
    pub any_data: Value<'a>,
    /// Map of string to an any type.
    /// This is not used for anything, but it is defined by AT-SPI.
    #[serde(borrow)]
    pub properties: HashMap<&'a str, Value<'a>>,
}

impl<T> Type for EventBody<'_, T> {
    fn signature() -> Signature<'static> {
        <(&str, i32, i32, Value, HashMap<&str, Value>)>::signature()
    }
}
TTWNO commented 8 months ago

Ah yes, it could be any "string" contained in the kind field. To deserialize events properly, we sometimes use substitution here, for example with State, but I'd also like to get this working with a new Operation enum for TextChangedEvent, or the key for the Property enum.

Often, this "string" is really an enum in disguise, sadly.

TTWNO commented 5 months ago

Revisiting this, since I've been looking for performance uplifts....

Should we do what zbus does and have T<'a> and TOwned = T<'static> types for events and such properties? I'm unsure if this always makes sense (i.e., Interfaces), but for the actual event body, item it occurred to, text String/&str types, etc, this could be viable.

I'm wondering if you'd be interested in trying to implement this? My suggestion would be to implement a EventBorrowed<'a> type and then run those benchmarks on it to see if there's any real benefit.

Looks like a lot of work, so no need to say yes... but I have noticed some of our tests are slow. A few ms each just to serialize/deserialize data (you can check with cargo-nextest) seems a little long, especially given what we know about event storms.

EDIT: I remember your benchmarks showed much better performance than a few ms per test, so maybe that's pure overhead from nextest's part?