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

put macros and traits of each type behind individual feature flags #70

Merged
merged 1 commit into from
May 12, 2024

Conversation

knickish
Copy link
Collaborator

@knickish knickish commented Jul 19, 2023

Closes #12. Leaving this in draft for a while, pending feedback for or against. Will try to keep PR up to date with current master.

Some timing results on my computer:

Binary Only (clean builds, approximate)

  • Avg. Debug: 0.8s
  • Avg. Release: 0.8s

All Default Features (clean builds, approximate)

  • Avg. Debug: 1.15s
  • Avg. Release: 1.35s

So ~30% speedup on debug builds and ~40% on release at the cost of a minor version bump

@knickish knickish force-pushed the feature_flags branch 6 times, most recently from d5aec99 to 3eb5cfb Compare July 19, 2023 13:15
@knickish knickish force-pushed the feature_flags branch 2 times, most recently from ccb6004 to 6cecef1 Compare August 6, 2023 16:25
@knickish
Copy link
Collaborator Author

knickish commented Aug 25, 2023

results from rust_serialization_benchmark on this branch vs. master:

log/nanoserde/serialize 
                        time:   [171.92 µs 172.05 µs 172.21 µs]
                        change: [-0.0851% +0.4384% +0.8537%] (p = 0.07 > 0.05)
                        No change in performance detected.
log/nanoserde/deserialize
                        time:   [1.8575 ms 1.8590 ms 1.8605 ms]
                        change: [+0.6107% +0.6935% +0.7688%] (p = 0.00 < 0.05)
                        Change within noise threshold.

mesh/nanoserde/serialize
                        time:   [873.69 µs 880.39 µs 890.72 µs]
                        change: [-25.831% -24.744% -23.606%] (p = 0.00 < 0.05)
                        Performance has improved.
mesh/nanoserde/deserialize
                        time:   [640.12 µs 641.81 µs 644.00 µs]
                        change: [-3.7967% -3.4917% -3.0225%] (p = 0.00 < 0.05)
                        Performance has improved.

minecraft_savedata/nanoserde/serialize
                        time:   [216.15 µs 216.79 µs 217.65 µs]
                        change: [-0.1880% +0.1268% +0.4970%] (p = 0.50 > 0.05)
                        No change in performance detected.
minecraft_savedata/nanoserde/deserialize
                        time:   [1.5320 ms 1.5411 ms 1.5502 ms]
                        change: [-0.1057% +0.1878% +0.4693%] (p = 0.20 > 0.05)
                        No change in performance detected.

mk48/nanoserde/serialize
                        time:   [895.38 µs 896.91 µs 898.61 µs]
                        change: [-3.1366% -2.8529% -2.5872%] (p = 0.00 < 0.05)
                        Performance has improved.
mk48/nanoserde/deserialize
                        time:   [2.5182 ms 2.5232 ms 2.5318 ms]
                        change: [-2.4276% -2.2242% -1.9036%] (p = 0.00 < 0.05)
                        Performance has improved.

Nothing super interesting, but some small improvements (probably due to code size reduction). I ran this forwards and backwards a couple of times, and the results were pretty consistent. For some reason the mesh serialization benchmark seems extremely sensitive to code size.

@not-fl3
Copy link
Owner

not-fl3 commented Aug 26, 2023

I still don't really have an opinion on this, the only input I can give - if I understand how cargo works correctly, than at least one library in the dependency tree with nanoserde = "0.1" will automatically enable all the formats for everyone.

So, in theory, having formats opt-in would make sense. However, personally, I would still prefer to have them all by default. Or I am just too used to just putting nanoserde = "0.1" for everythign? I really do not know :)

@knickish
Copy link
Collaborator Author

However, personally, I would still prefer to have them all by default

this is still the case in this PR, all of them are on by default, so hopefully would feel pretty much the same.

@knickish
Copy link
Collaborator Author

Made nanoserde_derive an optional dep so that toml can be used without it

@stefnotch
Copy link

I still don't really have an opinion on this, the only input I can give - if I understand how cargo works correctly, than at least one library in the dependency tree with nanoserde = "0.1" will automatically enable all the formats for everyone.

So, in theory, having formats opt-in would make sense. However, personally, I would still prefer to have them all by default. Or I am just too used to just putting nanoserde = "0.1" for everythign? I really do not know :)

I personally am in favour of this PR turning everything on by default. It immediately helps some people, and lets this remain a minor version bump for now.

Changing the defaults to be opt-in should ideally be done later, when an actual major version bump will happen.

@flukejones
Copy link
Contributor

What is the status on this? I especially would like to see it merged due to embedded system constraints.

@flukejones
Copy link
Contributor

Also agree with @stefnotch. The sane approach is all enabled by default. Folks who wish to disable parts will certainly do so.

@not-fl3
Copy link
Owner

not-fl3 commented May 10, 2024

What is the status on this? I especially would like to see it merged due to embedded system constraints.

If it actually solve some issue for you - I think we just need to wait for @knickish for a final approvement and get it merged :)

Any chance you want to help with the rebase to get it ready for the merge, @flukejones?

@knickish
Copy link
Collaborator Author

Sure, will try and get this rebased/fixed up and merged this weekend then

@flukejones
Copy link
Contributor

I've done fixup and PR to @knickish (knickish#7)

@knickish knickish force-pushed the feature_flags branch 2 times, most recently from 140ef2d to 98dede0 Compare May 12, 2024 16:50
@knickish knickish marked this pull request as ready for review May 12, 2024 16:52
@knickish knickish merged commit 1768216 into not-fl3:master May 12, 2024
16 checks passed
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.

Allow disabling support for unneeded serialization formats.
4 participants