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. #144

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Conversation

Grinkers
Copy link
Collaborator

@Grinkers Grinkers commented Jan 4, 2024

This is a breaking change to Serialize and edn-derive. See evaporei/edn-derive#37 for compatibility. Clean-history version of #137

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/144/files#diff-684ad70dd994bef2eaab407a0147b7b4ec3922c586fa15f9fe015d848abfc963L53-R54

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

This shows how expensive move vs borrow is (will continue to scale up depending on size).

@Grinkers Grinkers requested review from naomijub and evaporei January 4, 2024 00:14
@Grinkers Grinkers marked this pull request as ready for review January 4, 2024 00:21
@evaporei
Copy link
Collaborator

evaporei commented Jan 4, 2024

I think the performance improves because move does a drop, and deallocating memory usually is expensive.

@Grinkers
Copy link
Collaborator Author

Grinkers commented Jan 4, 2024

@naomijub @evaporei
As you can see in the last commit, we have a circular dependency issue. I'm open to suggestions on how to get out of the deadlock with releases.

@Grinkers
Copy link
Collaborator Author

Grinkers commented Jan 4, 2024

I think the performance improves because move does a drop, and deallocating memory usually is expensive.

In the example, it's not just dropping, but having to recreate it each loop too, essentially doing a clone() each time. it's about half of the cost in this benchmark.

@naomijub
Copy link
Owner

naomijub commented Feb 12, 2024

@Grinkers Those pending actions are not configured anywhere else, it might be from when the PR was created. Let me know when to merge and I can force merge it. Don't know how to clean it from the pipeline

I managed to fix them

@Grinkers
Copy link
Collaborator Author

I'm going to merge, with the TODO. It's needed because of the circular dependency (one of the two repos will need a temporary pointer. it makes sense to keep it here, as this is the one initiating the breaking change). After derive PRs, I'll clean it up as we progress.

@Grinkers Grinkers merged commit b48a802 into main Feb 12, 2024
28 checks passed
@Grinkers Grinkers deleted the no_move branch February 12, 2024 21:09
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.

3 participants