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

feat: Add (feature gated) derives for serde and rkyv #741

Closed
wants to merge 4 commits into from

Conversation

xdoardo
Copy link

@xdoardo xdoardo commented Nov 7, 2024

This small patch adds feature-gated derives of the serde::{Serialize, Deserialize} and rkyv::{Serialize, Deserialize, Archive} traits for the types defined in common.rs.

@xdoardo xdoardo marked this pull request as draft November 7, 2024 19:26
@xdoardo xdoardo marked this pull request as ready for review November 7, 2024 23:18
@philipc
Copy link
Contributor

philipc commented Nov 8, 2024

I'm reluctant to add this. These are not really types that users should be storing long term. They should only be used when calling APIs on object::write::Object, or for matching against when parsing. Some of them may be useful to store temporarily during the process of parsing or writing, but still not something that I would expect to be used for serialization.

What does this enable you to do? What alternatives have you considered? Do you need to be able to serialize all of these types? Why do you need both serde and rkyv?

@xdoardo
Copy link
Author

xdoardo commented Nov 8, 2024

Thanks for taking the time to respond! :)

What does this enable you to do? [...] Do you need to be able to serialize all of these types? Why do you need both serde and rkyv?

We want to use rkyv to serialize and deserialize compiled WASM modules in fashion that is, in most of the cases, faster than writing and reading proper object files. The top level type for this endeavour is SerializableCompilation, which uses the Relocation type.

We currently use our own definitions for most of the types involved, but I want to replace some of them - in particular Relocation and RelocationKind - with the ones from the object crate. As far as I can tell, for my specific needs deriving the traits for the two types above should suffice. I added the derivations for all the types in common.rs thinking in an "all or nothing" way since I did not know what your preferences would be, and I'm perfectly fine with adding the traits only for the types I need.

On the other hand, serde is used mostly for another endeavour: stopping and restarting running modules, and snapshots in general. For this, we actually have another possibility of achieving the same results so, in truth, I can see the need for the serde traits to be lifted relatively soon - we just haven't planned for it yet.

What alternatives have you considered?

I tried to implement the traits I need for the two types I mentioned above using the remote "tactics" from serde and rkyv, but it seems to me they aren't compatible with each other. Of course, I might be wrong and there might be a hacky (or not) way to make them work together! :)

@philipc
Copy link
Contributor

philipc commented Nov 8, 2024

We want to use rkyv to serialize and deserialize compiled WASM modules in fashion that is, in most of the cases, faster than writing and reading proper object files.

If you're not reading and writing proper object files, then it seems to me that the relocation types you use should have nothing to do with this crate anyway?

I want to replace some of them - in particular Relocation and RelocationKind - with the ones from the object crate.

This PR does not include Relocation (there isn't one in common.rs). Did you mean RelocationKind and RelocationEncoding. I'm not sure using those is a good idea. They intentionally do not cover all possible relocation types, instead only handling cases where some cross-format/architecture support is useful.

Additionally, if it's only those two types then I don't think its a big burden to define the types yourself and add a conversion between them if you actually need it. The fact that you only need two types makes me more reluctant to add these because it's still the same order of complexity for this crate in terms of features and testing while being a smaller benefit.

@xdoardo
Copy link
Author

xdoardo commented Nov 8, 2024

Alright, thanks again for taking the time to respond. I'll close the PR.

@xdoardo xdoardo closed this Nov 8, 2024
@xdoardo xdoardo deleted the derive-serde-rkyv branch November 8, 2024 09:46
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 this pull request may close these issues.

2 participants