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

Port ser::Serializer to io::Write #206

Merged
merged 1 commit into from May 4, 2020
Merged

Port ser::Serializer to io::Write #206

merged 1 commit into from May 4, 2020

Conversation

Plecra
Copy link
Contributor

@Plecra Plecra commented Mar 27, 2020

Makes the Serializer struct generic over io::Write, allowing it to be used with more types (and less allocation). As a side-effect, the trailing commas are also removed from the output.

This was a quick implementation, written in response to compiler messages, so it could probably do with some cleaning up. It might be a good time to approach #175

Closes #202

TODO

  • Update docs

Copy link
Collaborator

@kvark kvark left a comment

Thank you, great stuff! Just a few fixes needed

src/ser/mod.rs Outdated Show resolved Hide resolved
src/ser/mod.rs Outdated Show resolved Hide resolved
src/ser/mod.rs Show resolved Hide resolved
src/ser/mod.rs Outdated Show resolved Hide resolved
src/ser/mod.rs Outdated Show resolved Hide resolved
tests/129_indexmap.rs Show resolved Hide resolved
tests/depth_limit.rs Outdated Show resolved Hide resolved
@kvark
Copy link
Collaborator

kvark commented Apr 3, 2020

It's hard to look through your logic changes because of the formatting changes. Is there a way you could separate these in commits?

@Plecra
Copy link
Contributor Author

Plecra commented Apr 27, 2020

In case you didn't get a notification, I've separated the commits

Copy link
Collaborator

@kvark kvark left a comment

First commit review.
It's looks great in general!
Just one major concern to address.

tests/depth_limit.rs Outdated Show resolved Hide resolved
tests/preserve_sequence_ex1.ron Outdated Show resolved Hide resolved
@Plecra
Copy link
Contributor Author

Plecra commented Apr 27, 2020

So there were a couple things that came up while writing this that will need to be resolved:

ron/src/ser/mod.rs

Lines 178 to 185 in 92c1d5d

fn default_new_line() -> String {
#[cfg(not(target_os = "windows"))]
let new_line = "\n".to_string();
#[cfg(target_os = "windows")]
let new_line = "\r\n".to_string();
new_line
}

The serializer emits CRLF newlines on windows but the tests don't expect them, meaning the test quite can only be run on linux. This isn't going to be hard to solve ^^

assert_eq!(s, Ok(EXPECTED.to_string()));

At some point this test was broken, presumably by a PartialEq implementation for ron::Error going missing. Hopefully that can be added?

@kvark
Copy link
Collaborator

kvark commented Apr 27, 2020

I think this looks good now! Thank you!
Would you mind squashing everything up to the rustfmt commit?
Also, would you mind rebasing instead of introducing the merge commit at the end?

kvark
kvark approved these changes Apr 27, 2020
Copy link
Collaborator

@kvark kvark left a comment

Thank you!
bors r+

bors bot added a commit that referenced this pull request Apr 27, 2020
206: feat: port `ser::Serializer` to `io::Write` r=kvark a=Plecra

Makes the `Serializer` struct generic over `io::Write`, allowing it to be used with more types (and less allocation). As a side-effect, the trailing commas are also removed from the output.

This was a quick implementation, written in response to compiler messages, so it could probably do with some cleaning up. It might be a good time to approach #175 

Closes #202 

## TODO

- [ ] Update docs


Co-authored-by: Marli Frost <marli@frost.red>
@Plecra
Copy link
Contributor Author

Plecra commented Apr 27, 2020

Ah, I just remembered! I think we can avoid this being a breaking change by using String as a default for the generic parameter, and introducing a new constructor for generic writers. Either that, or we'll need to bump the version in Cargo.toml.

At this point, it's probably best to just make this 0.6.0. It is a pretty major change for users, and maintaining backwards compatibility with the current new constructor doesn't seem like a particularly high priority

@kvark
Copy link
Collaborator

kvark commented Apr 27, 2020

Yes, I agree, let's not try too hard to make this backwards compatible.

@bors
Copy link
Contributor

bors bot commented Apr 27, 2020

Timed out.

@kvark
Copy link
Collaborator

kvark commented Apr 27, 2020

The CI isn't happy - https://travis-ci.org/github/ron-rs/ron

@Plecra
Copy link
Contributor Author

Plecra commented Apr 27, 2020

It is not. I should've realized this was actually caused by my changes - using an io::Write meant including io::Error in the error type. Since it can't be Cloned or PartialEq'd, neither can ron::Error.

I've remade the original fix to the test.

assert_eq!(s.unwrap(), EXPECTED.to_string());

@Plecra Plecra changed the title feat: port ser::Serializer to io::Write WIP: port ser::Serializer to io::Write Apr 27, 2020
@Plecra Plecra changed the title WIP: port ser::Serializer to io::Write Port ser::Serializer to io::Write Apr 27, 2020
@kvark
Copy link
Collaborator

kvark commented May 2, 2020

Is this all ready? Could you rebase please?

@Plecra
Copy link
Contributor Author

Plecra commented May 4, 2020

Updated!

kvark
kvark approved these changes May 4, 2020
Copy link
Collaborator

@kvark kvark left a comment

Thank you!
bors r+

@bors
Copy link
Contributor

bors bot commented May 4, 2020

Build succeeded:

@bors bors bot merged commit 23649eb into ron-rs:master May 4, 2020
1 check passed
@nnethercote
Copy link
Contributor

nnethercote commented May 25, 2020

I've been using cargo-llvm-lines to look at compile times of webrender, in particular when the capture and replay features are on.

There was a 13% increase in the number of lines of LLVM IR generated with the capture feature enabled in webrender when going from ron 0.5.1 to 0.6.0. I'm pretty certain that this PR was the cause. This will have caused compile time regressions for webrender and other programs that use ron.

Here's some of the output from cargo-llvm-lines with 0.5.1:

  Lines        Copies         Function name
  -----        ------         -------------
1680955 (100%)  49965 (100%)  (TOTAL)
  75640 (4.5%)    620 (1.2%)  <&mut ron::ser::Serializer as serde::ser::SerializeStruct>::serialize_field
  28888 (1.7%)     92 (0.2%)  hashbrown::raw::RawTable<T>::rehash_in_place
  27980 (1.7%)   2771 (5.5%)  core::ptr::drop_in_place
  26712 (1.6%)    168 (0.3%)  hashbrown::raw::RawTable<T>::find
  23686 (1.4%)    409 (0.8%)  core::result::Result<T,E>::unwrap_or_else
  22344 (1.3%)    133 (0.3%)  <&mut ron::ser::Serializer as serde::ser::SerializeSeq>::serialize_element
  22091 (1.3%)    387 (0.8%)  core::option::Option<T>::map
  19084 (1.1%)    335 (0.7%)  core::option::Option<T>::ok_or_else
  18548 (1.1%)    128 (0.3%)  serde::ser::Serializer::collect_seq
  17296 (1.0%)     92 (0.2%)  hashbrown::raw::RawTable<T>::resize
  16893 (1.0%)    370 (0.7%)  core::ptr::swap_nonoverlapping_one

And here's the corresponding output with 0.6.0:

  Lines        Copies         Function name
  -----        ------         -------------
1897940 (100%)  50067 (100%)  (TOTAL)
 221960 (11.7%)    620 (1.2%)  <ron::ser::Compound<W> as serde::ser::SerializeStruct>::serialize_field
  40299 (2.1%)    133 (0.3%)  <ron::ser::Compound<W> as serde::ser::SerializeSeq>::serialize_element
  28888 (1.5%)     92 (0.2%)  hashbrown::raw::RawTable<T>::rehash_in_place
  28032 (1.5%)   2776 (5.5%)  core::ptr::drop_in_place
  26712 (1.4%)    168 (0.3%)  hashbrown::raw::RawTable<T>::find
  23686 (1.2%)    409 (0.8%)  core::result::Result<T,E>::unwrap_or_else
  22197 (1.2%)    389 (0.8%)  core::option::Option<T>::map
  20746 (1.1%)     82 (0.2%)  <ron::ser::Compound<W> as serde::ser::SerializeTuple>::serialize_element
  20724 (1.1%)    128 (0.3%)  serde::ser::Serializer::collect_seq
  19084 (1.0%)    335 (0.7%)  core::option::Option<T>::ok_or_else
  17296 (0.9%)     92 (0.2%)  hashbrown::raw::RawTable<T>::resize
  17052 (0.9%)     98 (0.2%)  <&mut ron::ser::Serializer<W> as serde::ser::Serializer>::serialize_newtype_variant
  16893 (0.9%)    370 (0.7%)  core::ptr::swap_nonoverlapping_one

This PR added the Compound type and logic for avoiding trailing commas, both of which increased the size of methods like serialize_field and serialize_element. These functions get instantiated 100s of times. I strongly suspect this PR is responsible for most or all of the increase.

If you want to try this yourself:

  • Install cargo-llvm-lines
  • Get a webrender clone.
  • Change the default features line in webrender/webrender/Cargo.toml to ["capture"].
  • Run cargo +nightly llvm-lines within webrender/webrender.

The non-obvious lesson here is that these methods can be instantiated very often, and so adding features/complexity can have surprisingly high costs.

@Plecra
Copy link
Contributor Author

Plecra commented May 25, 2020

Reviewing the changes, I think the regressions may be primarily due to the error handling and write_all inlining. It's probably not specializing it for Strings - we might be able to use a trait similar to serde_json's Read to force it to use a simpler implementation.

Doing that would also remove the awkward String::from_utf8 call.

@Plecra
Copy link
Contributor Author

Plecra commented May 25, 2020

cargo-llvm-lines is misbehaving on Windows, so I can't see the impact of the changes - Could you see what the output is like with this branch? https://github.com/Plecra/ron/tree/io-write

@nnethercote
Copy link
Contributor

nnethercote commented May 25, 2020

@Plecra: LLVM IR generation occurs before any inlining happens, so inlining isn't relevant. As I said above, I think the introduction of Compound and the extra trailing comma logic accounts for most of the change.

@panstromek
Copy link

panstromek commented Jun 12, 2020

@Plecra I added a workaround for the bug in cargo-llvm-lines you mentioned, so it should now work for Windows (just some error is logged, but you get the output).

@kvark kvark mentioned this pull request Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add function to serialize to a Writer
4 participants