Add and derive deserialize_from #1094

Gankra · 2017-11-16T18:29:16Z

The actual deserialize work is WIP, because I probably need to refactor it so it can be feature-flagged off. I think the first two commits are "good to go", though.

One thing missing from this design is deriving deserialize_from for exactly C-like enums, but that can obviously be done later.

Pushing this up now because my brain is melting trying to do the non-C-like enum work I need for webrender, and so I'm gonna take a bit of a "break" by dealing with review complaints.

Gankra · 2017-11-16T18:32:46Z

Oh also obviously BLATANTLY MISSING: TESTS. ANY TESTS.

Gankra · 2017-11-16T18:35:21Z

Any tips on testing this kind of thing would be appreciated. My local experiments have mostly consisted of asserting that serializing and deserializing round-trips correctly.

Gankra · 2017-11-16T19:46:26Z

Currently grinding through the serde test suite, lots of little derive bugs.

Starting to regret changing a bunch of things from -> Body to -> (Option<Common>, Body, Option<FromBody>) in order to prevent duplicating a static.

Gankra · 2017-11-17T05:04:49Z

Pushed fixes:

Fixed bad cfg's in builtin impl commit
Got existing test-suite passing with derive changes

One thing I didn't realize until now is that deserialize_from kinda doesn't make any sense when doing "remote" impls?

oli-obk · 2017-11-24T08:31:25Z

serde/src/de/impls.rs

@@ -66,13 +68,37 @@ impl<'de> Visitor<'de> for BoolVisitor {
    }
 }

+struct BoolFromVisitor<'a>(&'a mut bool);


I don't think primitive types need this specialization. The pointer indirection will probably be less performant than the default implementation. I'm easily convinced otherwise by benchmarks ;)

It's been a while since I implemented this, but I seem to recall getting wins from doing this. Gonna start profiling tomorrow.

oli-obk · 2017-11-24T08:32:39Z

serde/src/de/impls.rs

+        E: Error,
+    {
+        match str::from_utf8(v) {
+            Ok(s) => {


forward to visit_str instead of repeating the code

The deserialize() impl doesn't, and this code seems clearer without it. (happy to change if you disagree)

oli-obk · 2017-11-24T08:35:40Z

serde/src/de/impls.rs

+    where
+        D: Deserializer<'de>,
+    {
+        // Some enum is opaque, can't do anything good here :(


can't you do

match *self.0 { Some(ref mut data) => data.deserialize_from(deserializer), None => Some(T::deserialize(deserializer)?), }

Good point, but hard to say if this is justified. Very workload-specific.

oli-obk · 2017-11-24T08:37:07Z

serde/src/de/impls.rs

+                    where
+                        A: SeqAccess<'de>,
+                    {
+                        $clear(&mut self.0);


Isn't the real optimization here to not clear, but overwrite all already existing elements, if the new sequence has more elements, append, if it has less, drop the leftover ones?

You can't do this for BTreeSet and BinaryHeap, so I just did the generically correct thing. Seems like it could be left as future work to split this up and further optimize it?

dtolnay · 2017-11-28T19:42:59Z

I have fallen behind on PRs so I haven't had a chance to review the implementation yet, but -- I would be interested in exploring some ways to modularize the serde and serde_derive crates better so that large but uncommonly required features like this one can be delivered in their own crate. The serde_state / serde_derive_state feature is in a similar situation where it seems somewhat overwhelming to maintain as part of Serde but the community that wants the feature is willing to maintain it on the side.

Maybe this means factoring a lot of the serde_derive code into a separate pre-1.0 helper crate that is depended on by serde_derive, serde_derive_state, and a deserialize_from derive crate.

Would you be interested in figuring out a structure that would work for deserialize_from as part of getting this integrated? Otherwise would it be possible to provide deserialize_from in its own crate somehow, even if it means some code duplication with serde_derive?

Gankra · 2017-11-28T20:00:15Z

I don't really see how deserialize_from could be seperated out, realisitically? It's part of the trait definition.

oli-obk · 2017-11-29T08:14:01Z

I don't really see how deserialize_from could be seperated out, realisitically? It's part of the trait definition.

You can make a DeserializeFrom trait which is implemented for all Deserialize types

Gankra · 2017-11-29T19:38:33Z

How would that work? It can't be blanketed, because actual type-specific code needs to be emitted.

dtolnay · 2017-12-03T19:48:09Z

Okay I can see how shipping this outside of the Deserialize trait definition would be problematic.

I am on board with accepting this as long as it does not affect compile times for anyone not using deserialize_from (basically the same concern as for clone_from). As is, when I tried this it raises compile time for the serde crate by 13% (5.9 seconds to 6.7 seconds) which is a bit much for something that few people will use. Worse, Deserialize impls already compile very slow (serde-rs/json#313) so something like 2x more generated code in every impl would not be nice.

Could you propose what you think is the best way to feature gate this?

Up to you how you want to get this merged. If it is easier, we can merge the serde pieces up front and follow up in one or more other PRs with the serde_derive pieces as those are written and tested.

One thing I don't like here is all the deserialize_from impls that are conceptually no different from the default implementation. For example the bool one:

fn visit_bool<E>(self, v: bool) -> Result<(), E> {
    *self.0 = v;
    Ok(())
}

Could you share some benchmarks that show these outperform the default deserialize_from? Is there an easy way to tell which types benefit from a custom deserialize_from? Or is a custom deserialize_from always preferable?

Gankra · 2017-12-04T04:29:29Z

Ok finally got a chance to double check primitive stuff. Looks like, at least on webrender, removing the primitive impls is neutral, or possibly even profitable (the deserializer went from 7.1% to 6.6% of backend time; probably noise).

I'll take them out, then.

This adds a new "deserialize_from" feature (default off) that opts into deriving deserialize_from with #[derive(Deserialize)].

Gankra · 2017-12-04T18:35:52Z

Ok I refactored out all the deserialize_from derivation code out so it could be feature'd out. Ultimately we weren't sharing a ton of code, so it's not a huge loss. Writing it that way helped me incrementalize the development process for myself better, but that doesn't matter now.

I also did a kinda naive addition of deserialize_from to the test suite.

Gankra · 2017-12-06T20:39:53Z

Webrender, and therefore gecko, will likely be moving forward with a soft fork of serde in the next few days with these patches, plus the additional patch found on https://github.com/Gankro/serde/tree/deserialize_from_enums3

Even if you aren't ready to make a call on merging this, we'd certainly appreciate more thorough review. :)

oli-obk · 2017-12-07T07:16:59Z

I'd be on board with merging this PR as it is if we make the deserialize_from feature require nightly. Then we can play around with it a little without having to immediately stabilize it

Gankra · 2017-12-07T17:29:55Z

That makes sense to me.

Gankra · 2017-12-07T17:30:10Z

Wait, would that also put the trait method behind the feature?

Gankra · 2017-12-07T17:30:21Z

Also CC #855

oli-obk · 2017-12-07T21:40:34Z

Hmm right, that could be messy with derives and such :/

The custom one was functionally identical to the default implementation given by the Deserialize trait. If someone has benchmarks that the custom one performs better, we can put it back.

The naming here isn't great but no need to change it in this PR.

dtolnay · 2017-12-11T07:32:41Z

I started making progress on the review -- pushed a few fixup commits as I go. Some remaining items:

Before merging, I would like to get a sense of what this means for the future of csv::Reader and serde_json::StreamDeserializer. Is there a nice way to adapt these APIs to take advantage of deserialize_from? (I have not started thinking about this; it may turn out to be an easy answer.)
Benchmarks! It seems obvious that this would be faster in some usage patterns, but I would want to see at least one basic not-too-contrived benchmark that I can run using a real existing data format to see an improvement from this.
Currently for every struct with named fields we emit two totally identical copies of the __Field enum and the __FieldVisitor and its Visitor and Deserialize impls, which is a ton of redundant code. These should be emitted in a higher scope and shared by deserialize and deserialize_from. It is not critical to fix this before merging; will file an issue to follow up.

Gankra · 2017-12-11T07:39:14Z

I don't know what's interesting about those, I'll take a look in a bit.
I'll try to cut out some basic workloads. Where do you put benches?
Yes, I originally put in a fair bit of work trying to avoid this, but I backtracked on it when I deintegrated stuff to support cfg(). This ends up being pretty nasty to thread through the code for things like deserializing struct-like enum variants. Not clear it's worth the effort.

dtolnay · 2017-12-11T07:51:49Z

The CSV reader and JSON stream deserializer are interesting because they are APIs designed for deserializing lots of instances of the same type that are processed one at a time -- seems like an obvious candidate for deserialize_from. CSV is why we filed #855 in the first place so we may as well be certain that we are addressing their use case in the best way.

There aren't really benchmarks of Serde by itself since nothing interesting happens without a data format, but JSON benchmarks live here and Bincode benchmarks live here. I wouldn't bother with either of those, not looking for anything fancy just a 30 line thing I can run and see an improvement. I would do this myself but I am about to call it a night. 😴

Gankra changed the title ~~Add and derive deserialize_from~~ WIP: Add and derive deserialize_from Nov 16, 2017

Gankra force-pushed the deserialize_from branch from b86bbeb to 1a57e5d Compare November 17, 2017 05:01

oli-obk reviewed Nov 24, 2017

View reviewed changes

Gankra force-pushed the deserialize_from branch 2 times, most recently from 1e49cf2 to 108764b Compare December 4, 2017 18:22

Gankra changed the title ~~WIP: Add and derive deserialize_from~~ Add and derive deserialize_from Dec 4, 2017

Gankra added 4 commits December 4, 2017 13:23

Add deserialize_from to Deserialize

ab5e878

Augment builtin std/core Deserialize impls to implement deserialize_from

bc221ab

Derive deserialize_from for tuples and structs

e354dd0

This adds a new "deserialize_from" feature (default off) that opts into deriving deserialize_from with #[derive(Deserialize)].

test deserialize_from

34936be

Gankra force-pushed the deserialize_from branch from 108764b to 34936be Compare December 4, 2017 18:33

dtolnay added 2 commits December 10, 2017 21:27

Use the default deserialize_from for Option<T>

ffd2017

The custom one was functionally identical to the default implementation given by the Deserialize trait. If someone has benchmarks that the custom one performs better, we can put it back.

Hide deserialize_from

9641978

dtolnay added 8 commits December 10, 2017 22:02

Remove unused dev-dependency of serde_test on deserialize_from

a45f1ae

Enable deserialize_from in the test suite

a93f2eb

Can never see getters in a deserialize_from

61ca928

Do not emit deserialize_from if every field has deserialize_with

ccae35d

Less indentiation in deserialize_from_body

85e3ddc

Move all the deserialize_from derive code behind flag

d9e8949

More meaningful names and types for nop_reserve

3700779

Revert main_body naming change

9360094

The naming here isn't great but no need to change it in this PR.

dtolnay added 4 commits December 11, 2017 17:55

Remove need for allow(unreachable_code)

65104ac

Eliminate need for unwrap in deserialize_from_seq

13e7bee

Merge branch serde-rs/master into Gankro/deserialize_from

0c2e91f

Resolve conflict with pr 1115

0dac13e

dtolnay merged commit 0b89bc9 into serde-rs:master Dec 12, 2017

dtolnay mentioned this pull request Dec 12, 2017

Deserialize into a pre-existing struct #855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and derive deserialize_from #1094

Add and derive deserialize_from #1094

Gankra commented Nov 16, 2017

Gankra commented Nov 16, 2017

Gankra commented Nov 16, 2017

Gankra commented Nov 16, 2017

Gankra commented Nov 17, 2017

oli-obk Nov 24, 2017

Gankra Nov 29, 2017 •

edited

Loading

oli-obk Nov 24, 2017

Gankra Nov 29, 2017

oli-obk Nov 24, 2017 •

edited

Loading

Gankra Nov 29, 2017

oli-obk Nov 24, 2017

Gankra Nov 29, 2017

dtolnay commented Nov 28, 2017

Gankra commented Nov 28, 2017

oli-obk commented Nov 29, 2017

Gankra commented Nov 29, 2017

dtolnay commented Dec 3, 2017

Gankra commented Dec 4, 2017

Gankra commented Dec 4, 2017

Gankra commented Dec 6, 2017

oli-obk commented Dec 7, 2017

Gankra commented Dec 7, 2017

Gankra commented Dec 7, 2017

Gankra commented Dec 7, 2017

oli-obk commented Dec 7, 2017

dtolnay commented Dec 11, 2017

Gankra commented Dec 11, 2017

dtolnay commented Dec 11, 2017

Add and derive deserialize_from #1094

Add and derive deserialize_from #1094

Conversation

Gankra commented Nov 16, 2017

Gankra commented Nov 16, 2017

Gankra commented Nov 16, 2017

Gankra commented Nov 16, 2017

Gankra commented Nov 17, 2017

oli-obk Nov 24, 2017

Choose a reason for hiding this comment

Gankra Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

oli-obk Nov 24, 2017

Choose a reason for hiding this comment

Gankra Nov 29, 2017

Choose a reason for hiding this comment

oli-obk Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Gankra Nov 29, 2017

Choose a reason for hiding this comment

oli-obk Nov 24, 2017

Choose a reason for hiding this comment

Gankra Nov 29, 2017

Choose a reason for hiding this comment

dtolnay commented Nov 28, 2017

Gankra commented Nov 28, 2017

oli-obk commented Nov 29, 2017

Gankra commented Nov 29, 2017

dtolnay commented Dec 3, 2017

Gankra commented Dec 4, 2017

Gankra commented Dec 4, 2017

Gankra commented Dec 6, 2017

oli-obk commented Dec 7, 2017

Gankra commented Dec 7, 2017

Gankra commented Dec 7, 2017

Gankra commented Dec 7, 2017

oli-obk commented Dec 7, 2017

dtolnay commented Dec 11, 2017

Gankra commented Dec 11, 2017

dtolnay commented Dec 11, 2017

Gankra Nov 29, 2017 •

edited

Loading

oli-obk Nov 24, 2017 •

edited

Loading