reduce Visitor::finalize overhead
#1
Comments
scottlamb
changed the title
Visitor::finalize overhead
|
Here's an idea. Step 1: test out the Step 2: assuming so, use impl Deserialize for Foo {impl Deserialize for Foo {
fn deserialize(element: ElementReader<'_>) -> Result<Self, VisitorError> {
let mut out = MaybeUnit::uninit();
let mut builder = BlahVisitor { out: &mut out, uninitialized: 3 };
element.read_to(&mut builder)?;
::static_xml::de::check_uninitialized(self.uninitialized, &ELEMENTS[..])?;
Ok(unsafe { out.assume_init() })
}
}
struct BlahVisitor<'a> {
out: &'a mut MaybeUninit<Blah>,
uninitialized: u64, // bitmask
}
impl<'a> ElementVisitor for BlahVisitor<'a> {
fn element<'a>(&mut self, child: ElementReader<'a>) -> Result<Option<ElementReader<'a>>, VisitorError> {
match ::static_xml::de::find(&child.expanded_name(), ELEMENTS) {
Some(0usize) if (self.uninitialized & 1) != 0 => {
unsafe {
std::ptr::write(
self.out.as_mut_ptr().offset(offset_of!(Foo, field_a)) as *mut TypeA,
DeserializeField::init(child)?,
)
};
self.uninitialized &= ~1;
return Ok(None);
}
Some(0usize) if (self.uninitialized & 1) == 0 => {
let field = unsafe { self.out.as_mut_ptr().offset(offset_of!(Foo, field_a)) as *mut TypeA as &mut TypeA };
DeserializeField::append(field, child)?
}
// .. likewise for field_b
_ => Ok(Some(child)),
}
}
}The idea is that if all the individual fields are initialized, then the final struct should be also. I don't think padding needs to be initialized. Any optional members should be filled in with a default at the end rather than failing via This might be just moving the bloat to the |
|
Looking good. Implemented the The so, looks like:
Also, the interface is actually improving. I no longer need as many traits. Where I now have this (and likewise for attributes and flattens): static-xml/static-xml/src/de/mod.rs Lines 661 to 689 in ca51ec1 I can instead have something like this: pub trait DeserializeElementField {
/// Called on the first occurrence of this field's element within the parent.
fn init(element: ElementReader<'_>) -> Result<Self, VisitorError>;
/// Called on subsequent occurrences of this field's element within the parent.
///
/// `self` was previously returned by `init` and has been through zero or more prior `update` calls.
fn update(&mut self, element: ElementReader<'_>) -> Result<(), VisitorError>;
/// Called iff this field's element was not found within the parent and no default is set.
fn missing(expected: &ExpandedNameRef<'_>) -> Result<Self, VisitorError>;
}Fewer traits, IMHO more clear. I'm going to proceed with the |
scottlamb
added a commit
that referenced
this issue
Dec 16, 2021
|
I've got a messy, proof-of-concept implementation of the Note the Some(0usize) if self.choice_present => {
::static_xml::de::DeserializeElementField::update(
unsafe { &mut *unsafe { &raw mut (*self.out).choice } },
child,
)?;
Ok(None)
}
Some(0usize) => unsafe {
::std::ptr::write(
unsafe { &raw mut (*self.out).choice },
::static_xml::de::DeserializeElementField::init(child)?,
);
self.choice_present = true;
Ok(None)
},To improve this I'm going to look into the table-based approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Much of the bloat of the generated code is in the
*Visitor::finalizemethods, which transfer stuff from the builder (typically with types likeOption<T>) to the final product (with types likeT).Look at ways to reduce it.
My first attempt was to bypass this entirely, more or less as
yaserdedoes.#static_xml(direct)achieves this. But there are three downsides:Defaultimpl for everything, which I'd rather avoid.field: Trather thanfield: Option<T>, it can't error if a required field is missing or even distinguish between absent and default valueVec<T>doesn't work properly. It will discard all but the last occurrence. I haven't looked at the yaserde code, but I suspect it avoids this because it handles all the flattened stuff in a single call (in a gross way: it buffers everything by serializing all the unknown elements then deserializes them again). I use theDeserializeFlattenedinterface instead.Maybe there's some big thing I can avoid in the generated code now. Or maybe there's a middle ground in which everything still needs
Default(the first cavet holds) but theVisitortype is something like this:and just check those bools, or via a bitmask. Maybe this could even work with the table-driven approach.
The text was updated successfully, but these errors were encountered: