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

Make print_matrix O(1) again #23681

Merged
merged 1 commit into from
Sep 15, 2017
Merged

Conversation

iamed2
Copy link
Contributor

@iamed2 iamed2 commented Sep 12, 2017

Thanks @sbromberger for reporting this on Slack

Previously, printing an array A at the REPL would allocate O(sum(size(A))) memory. This change was made to support OffsetArrays but OffsetArrays appears to work with this new version too.

@iamed2
Copy link
Contributor Author

iamed2 commented Sep 12, 2017

Key benchmark:

display(rand(1800000000))

@iamed2
Copy link
Contributor Author

iamed2 commented Sep 12, 2017

I believe this commit introduced the performance regression: 0d688dd

@ararslan ararslan added display and printing Aesthetics and correctness of printed representations of objects. performance Must go faster labels Sep 12, 2017
@StefanKarpinski
Copy link
Member

So I guess the extra credit here would be either a test that takes a short time when this is O(1) but effectively hangs when this is O(sum(size(A)). Or a benchmark in BaseBenchmarks.

@iamed2
Copy link
Contributor Author

iamed2 commented Sep 13, 2017

I think the benchmark is the best choice but I've never done that before, so I'll have to read up on BaseBenchmarks.jl. I managed to find a call that calls print_matrix but sends everything to DevNull so I just need to add it.

@iamed2
Copy link
Contributor Author

iamed2 commented Sep 13, 2017

Well, I ran into a regression which I think is because of #22932

The vector and column matrix cases are faster and use less memory as expected. The square matrix case regresses. I think this is because in the matrix case, print_matrix calls (n - length(Ralign)) .+ colsA. My hunch is that previously Julia was smart about reusing the allocated array each time, as it was essentially just a copy of a Vector{Int} plus some Int.

Now that colsA is some linear indices, it makes sense to just shift the indices. Unfortunately + has been deprecated and .+ allocates an array. broadcast(+, (n - length(Ralign)), colsA) would have the intended behaviour but would be pretty ugly. Which do you think I should do?

@iamed2
Copy link
Contributor Author

iamed2 commented Sep 13, 2017

Now that regression is gone. Just noise I guess? I feel like that shouldn't be an issue with BenchmarkTools but oh well.

@mbauman mbauman merged commit c23d6bc into JuliaLang:master Sep 15, 2017
@iamed2 iamed2 deleted the print_matrix-speedup branch September 15, 2017 20:14
@sbromberger
Copy link
Contributor

Any possibility of a backport?

@mbauman mbauman mentioned this pull request Oct 6, 2017
ararslan pushed a commit that referenced this pull request Oct 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
display and printing Aesthetics and correctness of printed representations of objects. performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants