search

not-fl3 / nanoserde

Serialisation library with zero dependencies
689 stars 39 forks source link

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

Closed knickish closed 4 months ago

knickish commented 1 year ago

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)

All Default Features (clean builds, approximate)

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

knickish commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 9 months ago

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

stefnotch commented 5 months ago

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 commented 5 months ago

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

flukejones commented 5 months ago

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

not-fl3 commented 4 months ago

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 commented 4 months ago

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

flukejones commented 4 months ago

I've done fixup and PR to @knickish (https://github.com/knickish/nanoserde/pull/7/)