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

Allow serialising format_args! directly #786

Closed
clarfonthey opened this issue Feb 22, 2017 · 15 comments
Closed

Allow serialising format_args! directly #786

clarfonthey opened this issue Feb 22, 2017 · 15 comments

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented Feb 22, 2017

I've seen a fair number of Serialize implementations that involve calling to_string and I wonder if this extra allocation could be removed somehow. Take this example from chrono:

serializer.serialize_str(&format!("{:?}", self))

I wonder if we could perhaps modify this to:

serializer.serialize_str_format(format_args!("{:?}", self))

So that the serializer could call write itself directly to a buffer. For formats that don't require escaping characters, this could involve writing the data directly to the output stream, and for formats that do, this might involve writing to a temporary buffer (only allocated once) and then copying the data out with the escaped characters in it. Perhaps it could also default to calling format and then serialize_str if implementors choose to not specialise it.

Not sure how much of a performance gain this would be, but it seems worth investigating imo.

@clarfonthey
Copy link
Contributor Author

clarfonthey commented Feb 22, 2017

Possible specialisations for ease of use could be:

  • serialize_str_debug(val) -> serialize_str_format(format_args!("{:?}", val))
  • serialize_str_display(val) -> serialize_str_format(format_args!("{}", val))

@dtolnay
Copy link
Member

dtolnay commented Feb 22, 2017

Neat idea. The chrono impls could be optimized similar to this and I doubt there would be much performance left to gain after that, but it may make a difference in other cases. This ties in to #746 as well.

@dtolnay
Copy link
Member

dtolnay commented Feb 22, 2017

@nox would something like Serializer::collect_str<T: Display>(self, T) with a default impl do the trick?

@clarfonthey
Copy link
Contributor Author

Personally, I'd rather not limit this to just types with a Display impl because that requires a lot more boilerplate.

@nox
Copy link
Contributor

nox commented Feb 22, 2017

@clarcharr This is the only way I can think of of using the Display trait while not making a breaking change. What do you suggest?

@dtolnay
Copy link
Member

dtolnay commented Feb 22, 2017

Can you give an example of the boilerplate it would require? FormatArgs has a Display impl so serializer.collect_str(format_args!("{:?}", val)) would use the Debug impl of val.

@nox
Copy link
Contributor

nox commented Feb 22, 2017

Or do you just mean to support Debug too?

@nox
Copy link
Contributor

nox commented Feb 22, 2017

@dtolnay Actually it should probably be Serializer::collect_str<T: Display>(self, &T), no?

@clarfonthey
Copy link
Contributor Author

Oh wait, I wasn't aware that FormatArgs implemented Display. That works.

@clarfonthey
Copy link
Contributor Author

The type of boilerplate I was thinking of was that using wrapper types to manually implement Display could feel a lot like boilerplate, but if FormatArgs allows that then I think that it's fine to only require that for complex implementations.

@dtolnay
Copy link
Member

dtolnay commented Feb 24, 2017

Instead of basing this on Display, it may be a good opportunity to base it on a trait of our own that is able to provide both a human-readable string representation and a compact binary representation. This would make a lot of sense for chrono, IpAddr etc. The idea would be that JSON and YAML and TOML serialize as a string while Bincode serializes as bytes.

@clarfonthey
Copy link
Contributor Author

I mean, I think that there's value in having a minimal display trait for strings as well. Because the value sent to Display is usually intended to be human-readable, there are often more compact ways to display things.

One example off the top of my head is that an object containing dates might want to display them in a format other than ISO, whereas a serialised format would likely prefer ISO.

@nox
Copy link
Contributor

nox commented Feb 25, 2017

That seems like a job for #[serde(with)], orthogonal to a method for the Display trait.

nox added a commit to nox/serde that referenced this issue Feb 26, 2017
The default implementation collects the Display value into a String
and then passes that to Serializer::serialize_str when the std or collections
features are enabled, otherwise it unconditionally returns an error.
nox added a commit to nox/serde that referenced this issue Feb 26, 2017
The default implementation collects the Display value into a String
and then passes that to Serializer::serialize_str when the std or collections
features are enabled, otherwise it unconditionally returns an error.
nox added a commit to nox/serde that referenced this issue Feb 26, 2017
The default implementation collects the Display value into a String
and then passes that to Serializer::serialize_str when the std or collections
features are enabled, otherwise it unconditionally returns an error.
@dtolnay
Copy link
Member

dtolnay commented Feb 26, 2017

I filed #790 to follow up on the readable vs compact aspect of this in a separate change.

nox added a commit to nox/serde that referenced this issue Feb 27, 2017
The default implementation collects the Display value into a String
and then passes that to Serializer::serialize_str when the std or collections
features are enabled, otherwise it unconditionally returns an error.
@dtolnay dtolnay closed this as completed in a9a0535 Mar 6, 2017
dtolnay added a commit that referenced this issue Mar 6, 2017
Introduce Serializer::collect_str (fixes #786)
@dtolnay
Copy link
Member

dtolnay commented Apr 9, 2017

Fixed in #789.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants