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

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

Closed
wants to merge 4 commits into from

Conversation

Grinkers
Copy link
Collaborator

@Grinkers Grinkers commented Dec 23, 2023

This is a breaking change to Serialize and edn-derive. See evaporei/edn-derive#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
Copy link
Collaborator Author

Grinkers commented Dec 23, 2023

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
Copy link
Collaborator Author

Rebased in Grinkers#4 and part of #140

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.

1 participant