Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add DecodeAsType backed by Visitor impls for standard types. #11

Merged
merged 70 commits into from
Mar 13, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Feb 2, 2023

Part of a series:

  • scale-encode: new crate to handle encoding using type info.
  • !scale-decode: handle decoding using type info.
  • scale-value: update Value type to use scale-encode/decode.
  • subxt: Start taking advantage of these in Subxt.

This PR:

  • Makes the Visitor trait suport zero-copy decoding from SCALE bytes, and generally overhauls it to be simpler/more powerful to write impls.
  • Adds an IntoVisitor trait which describes how you can obtain a visitor impl for some type.
  • Adds a DecodeAsType trait which is implemented by default when the above is true (and the visitor error can convert into a crate::Error). This roughly mirrors EncodeAsType from scale-encode.
  • Adds a DecodeAsFields trait which is implemented for all composite/tuple types by default and allows a list of scale_info::Fields to be used to decode some value into a concrete type (this roughly mirrors EncodeAsFields from the scale-encode crate).
  • Adds a load of impls for common types
  • Adds a DecodeAsType proc macro that implements the necessary things on custom structs and enums.
  • Does various tidyup.

Closes #8

@jsdw jsdw mentioned this pull request Feb 2, 2023
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM! Amazing change!

jsdw added 3 commits March 3, 2023 15:28
…nabled, since we never actually check whether hiding things works (and in this cse we hit an issue anyway)
// and appending. This is used as part of our `Context`
// type and is not a part of the public API.
#[derive(Debug, Clone)]
pub struct LinkedList<T>(Option<Arc<Inner<T>>>);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand why you need a LinkedList here, isn't VecDeque sufficient?

It seems like it's just need push_front and iterator over it?

Copy link
Collaborator Author

@jsdw jsdw Mar 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right! I wrote the linkedlist back when I was passing context in, and so needed to be able to cheaply clone and add items to the front. But now, since it's just appended to on the way back, you're totally right and a simple Vec should do the trick!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got rid of LinkedList and replaced with a Vec; I'll do the same in scale-encode too!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// limitations under the License.

/// A quick hopefully-stack-only vec implementation for holding TypeIds.
pub struct StackVec<T, const N: usize> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would interesting with some benchmarks on this compared to Vec and SmallVec :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe yeah; I just wrote the minimum bit really to avoid another dependency; the hope is that it will never need to allocate unless something crazy comes up!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we'd want to squeeze every bit of performance out of it, the current implementation could allocate twice when moving the vector from the stack to the heap.

// First allocation goes here
                    let mut v = items[..].to_vec();
                    
// Potential allocation for extending the vector + a memcpy
                    v.push(item);
                    
                    self.inner = StackVecInner::Heap { items: v };

We could optimize those similar to:

            let mut v = Vec::with_capacity(N + 1);
            // SAFETY: we allocate the appropriate number of elements
            unsafe {
                items.as_ptr().copy_to_nonoverlapping(v.as_mut_ptr(), N);
                v.set_len(N);
                // We could probably do some pointer magic to call `set_len` only once
                v.push(item);
            }
            self.inner = StackVecInner::Heap { items: v };

Please feel free to ignore this :D Seems that may not be the most common case

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping it'll never happen; only if a compact encoded type is nested in a crazy number of new type wrappers :) so yeah it's no biggie, but nice one!

Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@jsdw jsdw merged commit 4d40391 into main Mar 13, 2023
@jsdw jsdw deleted the jsdw-decode-as-type branch March 13, 2023 11:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lifetimes are anon
3 participants