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

remove unbound type parameters #533

Merged
merged 1 commit into from
Nov 26, 2022
Merged

remove unbound type parameters #533

merged 1 commit into from
Nov 26, 2022

Conversation

nsajko
Copy link
Contributor

@nsajko nsajko commented Sep 8, 2022

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

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

codecov bot commented Sep 8, 2022

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%) ⬆️
Run.test 93.56% <50.00%> (-0.02%) ⬇️

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
Copy link
Member

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
Copy link
Contributor Author

nsajko commented Sep 12, 2022

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

@MasonProtter
Copy link
Member

@tkf could we get this merged?

@jishnub
Copy link

jishnub commented Nov 18, 2022

Gentle bump @tkf

@MasonProtter
Copy link
Member

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.

This was referenced Nov 26, 2022
@MasonProtter MasonProtter merged commit 121eef9 into JuliaFolds:master Nov 26, 2022
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.

3 participants