JuliaFolds / Transducers.jl

Efficient transducers for Julia
https://juliafolds.github.io/Transducers.jl/dev/
MIT License
433 stars 24 forks source link

remove unbound type parameters #533

Closed nsajko closed 1 year ago

nsajko commented 2 years ago

I didn't check, but unbound type parameters often cause performance issues, so this may not be merely cosmetic.

codecov[bot] commented 2 years ago

Codecov Report

Merging #533 (ee10e77) into master (6125fe0) will not change coverage. The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master     #533   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files          32       32           
  Lines        2227     2227           
=======================================
  Hits         2087     2087           
  Misses        140      140           
Flag Coverage Δ
Pkg.test 89.66% <50.00%> (+0.03%) :arrow_up:
Run.test 93.56% <50.00%> (-0.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/partitionby.jl 94.80% <0.00%> (ø)
src/processes.jl 94.21% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

MasonProtter commented 1 year ago

The change to _foldl_array saves us two allocations and a lot of time on small arrays which use cartesian indexing.

Before:

julia> @btime foldxl(+, $(rand(10, 10)'))
  108.862 ns (2 allocations: 32 bytes)
51.25795446141273

julia> @btime foldxl(+, $(rand(100, 100)'))
  8.937 μs (2 allocations: 32 bytes)
4991.2254605668095

After:

julia> @btime foldxl(+, $(rand(10, 10)'))
  59.523 ns (0 allocations: 0 bytes)
54.69668080313199

julia> @btime foldxl(+, $(rand(100, 100)'))
  8.900 μs (0 allocations: 0 bytes)
4997.031407479028

You can see this difference gets washed out somewhat quickly for large arrays, but it certainly does help. Good find!

nsajko commented 1 year ago

FTR this new Julia feature (try it out with the nightlies) can be used to find such issues: JuliaLang/julia#46608

MasonProtter commented 1 year ago

@tkf could we get this merged?

jishnub commented 1 year ago

Gentle bump @tkf

MasonProtter commented 1 year ago

Hey @nsajko I’ve finally got the test suite working. Could you rebase on Master? That should get all tests passing and then I can merge.