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

Plecra · 2020-03-27T23:55:23Z

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

kvark

Thank you, great stuff! Just a few fixes needed

src/ser/mod.rs

tests/129_indexmap.rs

tests/depth_limit.rs

kvark · 2020-04-03T19:32:04Z

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 · 2020-04-27T15:03:09Z

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

kvark

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

tests/depth_limit.rs

tests/preserve_sequence_ex1.ron

Plecra · 2020-04-27T19:51:56Z

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 ^^

ron/tests/depth_limit.rs

Line 58 in 92c1d5d

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 · 2020-04-27T20:02:45Z

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

Thank you!
bors r+

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 <[email protected]>

Plecra · 2020-04-27T20:29:31Z

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 · 2020-04-27T20:32:11Z

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

bors · 2020-04-27T21:43:46Z

Timed out.

kvark · 2020-04-27T21:49:06Z

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

Plecra · 2020-04-27T22:11:25Z

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.

ron/tests/depth_limit.rs

Line 58 in 7ec65de

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

kvark · 2020-05-02T22:26:03Z

Is this all ready? Could you rebase please?

Plecra · 2020-05-04T15:32:34Z

Updated!

kvark

Thank you!
bors r+

bors · 2020-05-04T15:40:18Z

Build succeeded:

nnethercote · 2020-05-25T03:51:53Z

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 · 2020-05-25T08:32:35Z

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 · 2020-05-25T09:37:38Z

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 · 2020-05-25T10:43:48Z

@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 · 2020-06-12T07:17:21Z

@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 requested changes Apr 2, 2020

View reviewed changes

kvark requested changes Apr 27, 2020

View reviewed changes

tests/depth_limit.rs Outdated Show resolved Hide resolved

tests/preserve_sequence_ex1.ron Outdated Show resolved Hide resolved

kvark approved these changes Apr 27, 2020

View reviewed changes

Plecra changed the title ~~feat: port ser::Serializer to io::Write~~ WIP: port ser::Serializer to io::Write Apr 27, 2020

Plecra changed the title ~~WIP: port ser::Serializer to io::Write~~ Port ser::Serializer to io::Write Apr 27, 2020

feat: port ser::Serializer to io::Write

bb2e690

kvark approved these changes May 4, 2020

View reviewed changes

bors bot merged commit 23649eb into ron-rs:master May 4, 2020

This was referenced May 25, 2020

Build time regression from the Compound type #239

Open

Pretty array formatting broken in 0.6.0 #240

Closed

SimonSapin mentioned this pull request May 29, 2020

Investigate possibly-pathological amounts of LLVM IR with cargo-llvm-lines servo/servo#26713

Open

kvark mentioned this pull request Jul 7, 2020

pretty printing regression #252

Closed

juntyr mentioned this pull request Sep 3, 2023

Current Serializer is hard to maintain #175

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Plecra commented Mar 27, 2020 •

edited

Loading

kvark left a comment

kvark commented Apr 3, 2020

Plecra commented Apr 27, 2020

kvark left a comment

Plecra commented Apr 27, 2020 •

edited

Loading

kvark commented Apr 27, 2020

kvark left a comment

Plecra commented Apr 27, 2020 •

edited

Loading

kvark commented Apr 27, 2020

bors bot commented Apr 27, 2020

kvark commented Apr 27, 2020

Plecra commented Apr 27, 2020 •

edited

Loading

kvark commented May 2, 2020

Plecra commented May 4, 2020

kvark left a comment

bors bot commented May 4, 2020

nnethercote commented May 25, 2020

Plecra commented May 25, 2020 •

edited

Loading

Plecra commented May 25, 2020

nnethercote commented May 25, 2020

panstromek commented Jun 12, 2020

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

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

Conversation

Plecra commented Mar 27, 2020 • edited Loading

TODO

kvark left a comment

Choose a reason for hiding this comment

kvark commented Apr 3, 2020

Plecra commented Apr 27, 2020

kvark left a comment

Choose a reason for hiding this comment

Plecra commented Apr 27, 2020 • edited Loading

kvark commented Apr 27, 2020

kvark left a comment

Choose a reason for hiding this comment

Plecra commented Apr 27, 2020 • edited Loading

kvark commented Apr 27, 2020

bors bot commented Apr 27, 2020

kvark commented Apr 27, 2020

Plecra commented Apr 27, 2020 • edited Loading

kvark commented May 2, 2020

Plecra commented May 4, 2020

kvark left a comment

Choose a reason for hiding this comment

bors bot commented May 4, 2020

nnethercote commented May 25, 2020

Plecra commented May 25, 2020 • edited Loading

Plecra commented May 25, 2020

nnethercote commented May 25, 2020

panstromek commented Jun 12, 2020

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

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

Plecra commented Mar 27, 2020 •

edited

Loading

Plecra commented Apr 27, 2020 •

edited

Loading

Plecra commented Apr 27, 2020 •

edited

Loading

Plecra commented Apr 27, 2020 •

edited

Loading

Plecra commented May 25, 2020 •

edited

Loading