nanoserde icon indicating copy to clipboard operation
nanoserde copied to clipboard

put macros and traits of each type behind individual feature flags

Open knickish opened this issue 2 years ago • 5 comments

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 avatar Jul 19 '23 12:07 knickish

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.

knickish avatar Aug 25 '23 20:08 knickish

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 :)

not-fl3 avatar Aug 26 '23 01:08 not-fl3

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 avatar Aug 26 '23 01:08 knickish

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

knickish avatar Dec 16 '23 16:12 knickish

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.

stefnotch avatar Apr 15 '24 14:04 stefnotch

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

flukejones avatar May 09 '24 08:05 flukejones

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

flukejones avatar May 09 '24 08:05 flukejones

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?

not-fl3 avatar May 10 '24 14:05 not-fl3

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

knickish avatar May 10 '24 15:05 knickish

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

flukejones avatar May 10 '24 21:05 flukejones