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

Implement RawValue type #480

Merged
merged 1 commit into from
Sep 20, 2018
Merged

Implement RawValue type #480

merged 1 commit into from
Sep 20, 2018

Conversation

srijs
Copy link
Contributor

@srijs srijs commented Sep 14, 2018

Implements the RawValue type described in #355. On this benchmark, using the RawValue type instead of deserializing/serializing via Value results in a 3-4x speed-up.

test deserialize_and_serialize_raw_value ... bench:       1,759 ns/iter (+/- 354)
test deserialize_and_serialize_value     ... bench:       6,091 ns/iter (+/- 1,776)

I did not adhere fully to the way that the existing arbitrary precision feature is implemented, because I found it easier to use newtypes rather than structs to pass the raw data back and forth. If you want, I'm happy to either change it so that it uses the same approach as the arbitrary precision feature, or to change the arbitrary precision feature to use this (imo) simpler approach.

In the documentation for RawValue I mentioned two important caveats which apply for this implementation:

  1. When serializing, a RawValue will retain its original formatting and will not be minified or pretty-printed.
  2. When deserializing, RawValue can not be used with the #[serde(flatten)] attribute, as it relies on the original input buffer.

What you described in #323 might be a cleaner solution to these problems, but it also sounds more complex and resource hungry (using 4x-8x the amount of memory).

Let me know what you think!

@alteous
Copy link

alteous commented Sep 15, 2018

Awesome! Thank you. 👍

serde_json::from_str::<Wrapper>(r#"{"a": 1, "b": {"foo": 2}, "c": 3}"#).unwrap();
assert_eq!(r#"{"foo": 2}"#, wrapper_from_str.b.as_ref());

let wrapper_from_reader = serde_json::from_reader::<_, Wrapper<'static>>(
Copy link
Member

Choose a reason for hiding this comment

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

This is a compiler bug, this line shouldn't compile. I filed rust-lang/rust#54302 to follow up. Still thinking about what that means for how we want to expose RawValue.

Copy link
Contributor Author

@srijs srijs Sep 19, 2018

Choose a reason for hiding this comment

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

Hah, of course, not sure what I thought this would do!

Does that mean we might want to have an owned and a borrowed variant of RawValue?

Copy link
Member

Choose a reason for hiding this comment

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

I believe so. I opened #485 to experiment with that design. Let me know what you think.

@dtolnay
Copy link
Member

dtolnay commented Sep 20, 2018

I did not adhere fully to the way that the existing arbitrary precision feature is implemented, because I found it easier to use newtypes rather than structs to pass the raw data back and forth. If you want, I'm happy to either change it so that it uses the same approach as the arbitrary precision feature, or to change the arbitrary precision feature to use this (imo) simpler approach.

The way Number does it is more robust than newtypes when mixing with data formats that are not JSON. See 7e373b6.

@srijs
Copy link
Contributor Author

srijs commented Sep 20, 2018

Ah, I was looking for an explanation like that, might be worth adding a code comment to make this clear to anyone coming across this in the future.

As I said on the other PR (#485), do let me know if I can help move this forward in any way!

@dtolnay dtolnay merged commit 6d38232 into serde-rs:master Sep 20, 2018
@shepmaster shepmaster mentioned this pull request Sep 20, 2018
@srijs srijs deleted the feat/raw-value branch September 26, 2018 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants