naomijub / edn-rs

[DEPRECATED]: Crate to parse and emit EDN
https://crates.io/crates/edn-rs
MIT License
81 stars 14 forks source link

trait Serialize uses reference to self instead of move. #137

Closed Grinkers closed 10 months ago

Grinkers commented 10 months ago

This is a breaking change to Serialize and edn-derive. See https://github.com/edn-rs/edn-derive/pull/37 for compatibility.

Rationale: We want to be able to do things like

// assume some valid struct `val`
let s1 = val.serialize();
val.foobar = 42;
let s2 = val.serialize();

It is both annoying and potentially very expensive to move the struct, deallocate, and then create a whole new one each time you may want to serialize. If your Serializable data is inside of another struct, that requires a huge amount of work (making sure things can be Cloned, do deep copies, etc just to free it immediately after. In embedded this is probably critical, as you'll need the original, clone, and string data before starting to clean up, so max memory usage will high.

As far as performance goes https://github.com/edn-rs/edn-rs/pull/137/files#diff-684ad70dd994bef2eaab407a0147b7b4ec3922c586fa15f9fe015d848abfc963

edn                  time:   [2.8078 µs 2.8699 µs 2.9567 µs]
                        change: [-46.116% -44.053% -41.986%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 15 outliers among 100 measurements (15.00%)
  6 (6.00%) high mild
  9 (9.00%) high severe

Take this benchmark with a grain of salt, I'm currently on an old laptop. This shows how expensive move vs borrow is (will continue to scale up depending on size)

Grinkers commented 10 months ago

With this change, I also removed edn::to_string() in the last commit. Changing to a ref already is a breaking change to this function and it's needless api surface, doc bloat, etc just to .serialize(). Now there's an idiomatic and easy way to serialize, chain functions, etc. It's also confusing naming with the trait ToString which uses the same name.

Grinkers commented 10 months ago

Rebased in https://github.com/Grinkers/edn-rs/pull/4 and part of #140