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

Reduce allocations in _block_indices #88

Merged
merged 2 commits into from
May 13, 2022

Conversation

mcognetta
Copy link
Contributor

@mcognetta mcognetta commented Apr 22, 2022

_block_indices allocates two arrays that are the size of the number of blocks, no matter what the inputs are. This is quite slow, as it is done every time getindex, setindex, etc is called. It also makes a third allocation (calling sum on a slice of one of the aforementioned allocated arrays) but the size of this depends on the inputs.

This PR removes those allocations.

julia> x = [rand(5:30) for _ in 1:100]; y = Random.shuffle(x);

julia> z = BlockDiagonal([rand(x,y) for (x,y) in zip(x, y)]);

# before
julia> @allocated z[3, 3]
1888

# this PR
julia> @allocated z[3, 3]
96

@codecov
Copy link

codecov bot commented Apr 22, 2022

Codecov Report

Merging #88 (db99cc6) into master (0b98861) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   96.09%   95.87%   -0.23%     
==========================================
  Files           5        5              
  Lines         333      315      -18     
==========================================
- Hits          320      302      -18     
  Misses         13       13              
Impacted Files Coverage Δ
src/blockdiagonal.jl 85.48% <100.00%> (-1.09%) ⬇️
src/linalg.jl 95.18% <0.00%> (-0.23%) ⬇️
src/base_maths.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b98861...db99cc6. Read the comment docs.

Copy link
Collaborator

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! And apologies for the delay, it somehow slipped through my inbox. Added a suggestion, and if you could bump the version please then we are ready to merge.

src/blockdiagonal.jl Outdated Show resolved Hide resolved
src/blockdiagonal.jl Outdated Show resolved Hide resolved
@mzgubic mzgubic merged commit e471b23 into JuliaArrays:master May 13, 2022
@mcognetta
Copy link
Contributor Author

Ah, sorry I didn't see the comments. Thanks for handling it!

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.

2 participants