-
Notifications
You must be signed in to change notification settings - Fork 29
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
Replacement of Value and Bins with Writable/Readable derive macros #138
base: async
Are you sure you want to change the base?
Conversation
Currently reads return a |
Hi @jonas32, may I confirm that the purpose of this patch is to directly deserialize from the wire format (and serialize to the wire format) rather than using the Value enum? If so, it looks like exactly what I need. |
Hey @CMoore-Darwinium, yes. Goal is to directly build structs from the wire and write to it without intermediate steps and as low redundant allocations as possible. In the end, the target would be to more or less deprecate |
That's exactly what I've been hoping for. Profiling shows that creating and destroying aerospike::Value is our current limiting factor. Just playing devil's advocate here and I see you've already invested a lot of time into this patch. You've got these traits, ReadableValue / WritableValue that interact directly with PreParsedValue, which is essentially aerospike's wire format. I was wondering if we could use serde::Serialize and serde::Deserialize instead. That way implementations of serde::Deserializer and serde::Serializer, MapAccess, SeqAccess, etc could be provided for structpack, CDT, bins, formats etc could be passed as required. It would have the advantages:
If you're open to the idea, I would be able to help with the coding. |
i thought about this too initially. But there are a lot of tricky details with the wire format that might become hard to handle without working in the scope of the whole buffer. Lists/Maps are a good example, where you basically get X amount of bins with the same name and X amount of entries inside each and then have to join them together after parsing both. And on top, a different encoding for CDT returns. Not minding about particle type would also be hard, because that often has implications on the way values are encoded. Also having serde that deep inside of the client would build a big dependency on that crate, since there would be no way to interact with the Server outside of serde. Is it easily possible to add additional magic sugar for client specific things? Like ordered maps, geojson, hll etc.? In general I'm still open for that idea if it does not impact on performance/allocations. But i would like to hear @khaf's opinion on that first before i start digging into that. |
With Lists / Map serde works quite well. The particle type would be used. The idea is, the particle would be used in the deserializer which would call the right function on a Visitor specific to the type being destreamed https://docs.rs/serde/latest/serde/de/trait.Visitor.html the type itself would be responsible for either implementing or not implementing the appropriate visit function depending on whether the streamed data type makes sense to what you're streaming into. Serde itself has no dependencies. Its mostly just a set of traits that gets compiled down to essentially nothing so I don't think there is a huge problem in introducing it. Magic sugar for geojson and hll is more of a concern. I'll need to look into that. Some serde deserializers have their own special target types but it might be hacky. It could certainly be made to work, but finding a clean way might be harder. Serde does not allocate apart for a few very specific situations. Flattening structures is the only one I can think of off hand. It is mostly just a few layers of generics without much actual logic actually happening. |
Sounds good. |
New PR instead of #126 with the actual changes. I think its time to close the other one.
Writable:
Readable:
Client Update:
Exp/Op:
CDT: