awakesecurity / proto3-wire

https://hackage.haskell.org/package/proto3-wire
Other
23 stars 30 forks source link

Restructure map creation, and control inlining for better performance #84

Closed jcarr-awake closed 2 years ago

jcarr-awake commented 2 years ago

This is technically Breaking due to the removal of toMap. It could continue to be supplied as a deprecated combinator.

I was able to add an extra test at https://github.com/jcarr-awake/proto3-wire/tree/better-inlining-test but exporting the function creates some problems with overall inlining that are awkward to handle (and necessitates exporting an internal function).

New: benchmarking Parse int tree time 452.4 μs (451.7 μs .. 453.4 μs) 1.000 R² (1.000 R² .. 1.000 R²) mean 453.6 μs (453.0 μs .. 454.5 μs) std dev 2.535 μs (1.850 μs .. 3.313 μs)

benchmarking Parse int rose tree time 876.4 ns (872.2 ns .. 880.4 ns) 1.000 R² (1.000 R² .. 1.000 R²) mean 876.3 ns (872.2 ns .. 881.4 ns) std dev 9.572 ns (7.935 ns .. 11.66 ns)

Old: benchmarking Parse int tree time 514.9 μs (513.2 μs .. 516.7 μs) 1.000 R² (1.000 R² .. 1.000 R²) mean 515.7 μs (514.6 μs .. 516.8 μs) std dev 3.597 μs (2.999 μs .. 4.325 μs)

benchmarking Parse int rose tree time 1.065 μs (1.058 μs .. 1.072 μs) 1.000 R² (1.000 R² .. 1.000 R²) mean 1.064 μs (1.060 μs .. 1.069 μs) std dev 9.801 ns (4.830 ns .. 16.08 ns)

Geometric Mean Speedup: 17.7%

I saw a good speedup in our internal project from the previous improvement, so I'd expect to see similar here.

Gabriella439 commented 2 years ago

What do you mean when you say that this change no longer exports toMap? I don't see a change to the export list

jcarr-awake commented 2 years ago

Ah I missed a push, one second

Gabriella439 commented 2 years ago

I think it's fine to publish a breaking change to remove a utility that was intended for internal use