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

Make zvariant::Signature completely static & turn Type::signature to trait const #882

Closed
zeenix opened this issue Jul 1, 2024 · 3 comments · Fixed by #981
Closed

Make zvariant::Signature completely static & turn Type::signature to trait const #882

zeenix opened this issue Jul 1, 2024 · 3 comments · Fixed by #981
Labels
api break optimization zvariant Issues/PRs related to zvariant crate
Milestone

Comments

@zeenix
Copy link
Contributor

zeenix commented Jul 1, 2024

When I wrote zvariant, I was still very new to Rust and didn't make the best choices in some regard. Recently I looked into the postcard-rpc code, and that project also seem to have something very similar to a signature: a Schema. However, unlike our Signature type, Schema is completely static and allocation free (it's targeted for microcontrollers). So it got me thinking, if our Signature was similar, we could not only avoid a lot of allocations, but greatly optimize the ser/de process (according to the last benchmarks I conducted, signature parsing is what costs us the most, especially for array types). So something like this:

#[derive(Debug, Clone, Copy)]
pub enum Signature {
    // Simple types
    U8,
    Bool,
    I16,
    U16,
    I32,
    U32,
    I64,
    U64,
    F64,
    Str,
    Signature,
    ObjectPath,
    Value,
    #[cfg(unix)]
    Fd,

    // Container types
    Array {
        child: &'static Signature,
    },
    Dict {
        key: &'static Signature,
        value: &'static Signature,
    },
    Structure {
        fields: &'static [Signature],
    },
    #[cfg(feature = "gvariant")]
    Maybe {
        child: &'static Signature,
    },
}

pub trait Type {
    const SIGNATURE: Signature;
}

impl<T: Type> Type for [T] {
    const SIGNATURE: Signature = Signature::Array {
        child: &T::SIGNATURE,
    };
}

impl<T: Type> Type for (T,) {
    const SIGNATURE: Signature = Signature::Structure {
        fields: &[T::SIGNATURE],
    };
}
// TODO: Use a macro for for generating all tuple impls

All the above code works. 🍒 on top if we can make this work too:

impl Signature {
    pub const fn as_str(&self) -> &'static str {
        match self {
            Signature::U8 => "y",
            Signature::Bool => "b",
            Signature::I16 => "n",
            Signature::U16 => "q",
            Signature::I32 => "i",
            Signature::U32 => "u",
            Signature::I64 => "x",
            Signature::U64 => "t",
            Signature::F64 => "d",
            Signature::Str => "s",
            Signature::Signature => "g",
            Signature::ObjectPath => "o",
            Signature::Value => "v",
            #[cfg(unix)]
            Signature::Fd => "h",
            Signature::Array { child } => {
                concat_const::concat!("a", child.as_str())
            }
            Signature::Dict { key, value } => {
                concat_const::concat!("a{", key.as_str(), value.as_str(), "}")
            }
            Signature::Structure { fields } => {
                let fields = fields
                    .iter()
                    .map(|f| f.as_str())
                    .collect::<[]>()
                    .join("");
                &format!("({})", fields)
            }
            #[cfg(feature = "gvariant")]
            Signature::Maybe { child } => {
                let child = child.as_str();
                concat_const::concat!("m{}", child)
            }
        }
    }
}

but I suspect it's not possible and one part that will require allocation. However, with Display implementation, we can support writing to fixed sized buffers too.

Another possible hurdle could be zvariant::DynamicType. Maybe there could also be a DynamicSignature and/or Signature could have lifetime. Something I need to figure out..


Of course, this all would mean breaking API.

@zeenix zeenix added api break optimization zvariant Issues/PRs related to zvariant crate labels Jul 1, 2024
@zeenix
Copy link
Contributor Author

zeenix commented Jul 5, 2024

After a lot of different approaches, I finally came to a solution that works and I'm happy with:

https://github.com/zeenix/experiments/blob/a6b790a4619eacd5885111c8f4bbe371f36cec62/static-dbus-signature/src/signature.rs
https://github.com/zeenix/experiments/blob/a6b790a4619eacd5885111c8f4bbe371f36cec62/static-dbus-signature/src/type.rs
https://github.com/zeenix/experiments/blob/a6b790a4619eacd5885111c8f4bbe371f36cec62/static-dbus-signature/src/dynamic_type.rs

It's a nice compromise where Type impls (which is most of the cases) always have a completely const signature but anything needing dynamic signature needs to allocate. Perhaps it would be best to use Arc<[Signature]> but that's a detail.

I'll need to check signature use/needs in the code base to see if this will work for zbus needs but I'm feeling confident.

@zeenix
Copy link
Contributor Author

zeenix commented Sep 2, 2024

Quoting from #966

This PR addresses most of #882. Completely addressing it would require breaking API so it has to wait for the next major release.

Actually, I realized that this isn't true. The only reason Type::parsed_signature can't be a const, is that the Type macro needs to parse its string form (if user provides the signature through the attribute) and currently that can't be done because of circular dep it will create between zvariant and zvariant-derive.

This makes it important to do this already before the next 3.0 release. Here is the rough TODO:

  • Move parsed::Signature to zvaraint_utils and zvariant re-exports it, w/ zvaraint traits implemented on top.
  • zvariant_derive::Type
    • uses zvaraint_utils::parsed::Signature to parse the signature string.
    • translates the parsed form into tokens.
  • Conversion of Type::parsed_signature to const SIGNATURE: &'static parsed::Signature
  • remove Type::signature manual impls from the code base
  • use const constructors of parsed::Signature where possible
  • Update docs (methods and Type itself)

@zeenix
Copy link
Contributor Author

zeenix commented Sep 2, 2024

This PR addresses most of #882. Completely addressing it would require breaking API so it has to wait for the next major release.

Actually, I realized that this isn't true. The only reason Type::parsed_signature can't be a const, is that the Type macro needs to parse its string form (if user provides the signature through the attribute) and currently that can't be done because of circular dep it will create between zvariant and zvariant-derive.

oh actually, no. 😆 The other reason is that adding a const w/o a default value to public trait (Type) would a breaking change and the default value will have to parse the return value of signature() and we can't do that in a const context. Hence this change, while not too difficult (probably will only take a day), is still going to be a breaking change.

@zeenix zeenix modified the milestones: zvariant-5, zbus-5.0 Sep 6, 2024
zeenix added a commit to zeenix/zbus that referenced this issue Sep 9, 2024
named `SIGNATURE`. We also remove `Type::signature` method, since
`parsed::Signature` can be easily converted to a `crate::Signature`.
This commit makes similar changes to `DynamicType` and
`DynamicDeserialize` as well.

This is an API break that breaks all code that implements `Type`
(manually), `DynamicType` and/or `DynamicDeserialize`. In return, we
eliminate a lot of allocations, which can make a big difference when it
comes to (de)serialization on low-end machines.

Fixes dbus2#882.
zeenix added a commit to zeenix/zbus that referenced this issue Sep 9, 2024
Convert `Type::parsed_signature` to a const, named `SIGNATURE`. We also
remove `Type::signature` method, since `parsed::Signature` can be easily
converted to a `crate::Signature`. This commit makes similar changes to
`DynamicType` and `DynamicDeserialize` as well.

This is an API break that breaks all code that implements `Type`
(manually), `DynamicType` and/or `DynamicDeserialize`. In return, we
eliminate a lot of allocations, which can make a big difference when it
comes to (de)serialization on low-end machines.

Fixes dbus2#882.
zeenix added a commit to zeenix/zbus that referenced this issue Sep 9, 2024
Convert `Type::parsed_signature` to a const, named `SIGNATURE`. We also
remove `Type::signature` method, since `parsed::Signature` can be easily
converted to a `crate::Signature`. This commit makes similar changes to
`DynamicType` and `DynamicDeserialize` as well.

This is an API break that breaks all code that implements `Type`
(manually), `DynamicType` and/or `DynamicDeserialize`. In return, we
eliminate a lot of allocations, which can make a big difference when it
comes to (de)serialization on low-end machines.

Fixes dbus2#882.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api break optimization zvariant Issues/PRs related to zvariant crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant