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

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

Closed
luukvanderduim opened this issue Mar 7, 2024 · 2 comments · Fixed by #201
Closed

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

luukvanderduim opened this issue Mar 7, 2024 · 2 comments · Fixed by #201

Comments

@luukvanderduim
Copy link
Collaborator

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()
	}
}
@luukvanderduim luukvanderduim changed the title A 'zero-copy' EventBody<'a, T> without custom deserializers? A 'zero-copy' EventBody<'a, T> without custom deserializer? Mar 7, 2024
@TTWNO
Copy link
Member

TTWNO commented Mar 7, 2024

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
Copy link
Member

TTWNO commented Jun 18, 2024

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?

@TTWNO TTWNO linked a pull request Jun 27, 2024 that will close this issue
@TTWNO TTWNO closed this as completed in ecbd191 Jun 27, 2024
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 a pull request may close this issue.

2 participants