Skip to content

Conversation

@alyst
Copy link
Contributor

@alyst alyst commented May 18, 2017

Following up the discussion at #21564, replace a = Vector{T}(n) with resize!(a, n).
Hopefully should decrease resources usage by a tiny bit.

Also add comments clarifying why BLAS routine is called twice.

@andreasnoack andreasnoack changed the title BLAS wrappers: use resize!(a) instead of a = Vector{T}() LAPACK wrappers: use resize!(a) instead of a = Vector{T}() May 18, 2017
@ararslan
Copy link
Member

Dunno how visible this will be to the Base benchmarks, but...

@nanosoldier runbenchmarks(ALL, vs=":master")

@ararslan ararslan added linear algebra Linear algebra performance Must go faster labels May 18, 2017
iwork = Vector{BlasInt}(1)
liwork = BlasInt(-1)
rwork = Array{$relty,0}()
rwork = Vector{$relty}(1)
Copy link
Member

@Sacha0 Sacha0 May 18, 2017

Choose a reason for hiding this comment

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

Why change from a zero-dimensional Array to a single-element one-dimensional Array? Ah, I see rwork was formerly bound to a fresh Vector below, and you now resize instead. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

this commit should probably be squashed, on merge if not before

Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

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

Thanks @alyst! :) (Absent objections, requests for time, or someone beating me to it, I plan to merge this tomorrow morning.)

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@ararslan
Copy link
Member

Those benchmarks are a little surprising given that the intent of the PR is to reduce memory usage.

@alyst
Copy link
Contributor Author

alyst commented May 18, 2017

Oh, well. Maybe resize!(A, n) is a little bit eager than Vector(n) and allocates a little bit more than requested (and more than the ctor)?
It calls jl_array_grow_end() internally. Maybe there's way to be more precise.

@alyst
Copy link
Contributor Author

alyst commented May 18, 2017

It looks, though, that time-wise these linalg benchmarks are much more precise than their tolerance values: all changes are within 3%.
FWIW, there's 11 that are "faster" and only 2 that are "slower" :)

@ararslan
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@fredrikekre
Copy link
Member

fredrikekre commented May 18, 2017

Last commit (change of array growth) seems like a change for another PR? Ref #8269 and #16305

@alyst
Copy link
Contributor Author

alyst commented May 18, 2017

@fredrikekre Absolutely. Just a wild guess. OTOH it shouldn't be as disruptive as the change of the growth rate.

@alyst
Copy link
Contributor Author

alyst commented May 18, 2017

Funnily, Travis failed in the QR test, but the problem is not in the linalg. Most likely the last commit exposed something in string interpolation.
The errors come from test/lq.jl:

lstring = sprint(show,l)
qstring = sprint(show,q)
@test sprint(show,lqa) == "$(typeof(lqa)) with factors L and Q:\n$lstring\n$qstring"

The difference between LHS and RHS is the last lstring char. In one of the failing tests LHS contains -0.0824939+0.344439im]\nComplex{Float64}[, whereas RHS fragment is -0.0824939+0.344439im\x01\nComplex{Float64}[.

@ararslan ararslan requested a review from yuyichao May 18, 2017 23:01
@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@alyst
Copy link
Contributor Author

alyst commented May 19, 2017

Surprisingly, the benchmarks had not regressed memory-wise except join(string) (and I would not call join(string) a regression, because the memory allocation very much depends on the lengths of concrete strings being joined).
The LAPACK memory regressions are gone.
Could we ask nanosoldier to run the benchmarks once again to be more sure about time-wise changes?

@KristofferC
Copy link
Member

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

src/array.c Outdated
size_t nbinc = inc * elsz;
size_t newlen = a->maxsize == 0 ? (inc < 4 ? 4 : inc) : a->maxsize * 2;
while ((n + inc) > newlen - a->offset)
// if the requested size is much larger than the current maxsize,
Copy link
Member

Choose a reason for hiding this comment

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

While this heuristic doesn't sound absurd to me, maybe it would make sense (as in more robust) to add an exact=false argument to resize!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think giving the user more control over memory allocation, resize!() in particular, would make a lot of sense. Unfortunately, I didn't expect that trivial LAPACK fixes would require that sort of things: the last commit was just a quick check. I'd rather way until the memory allocation problem is addressed by a separate PR and update this one accordingly.

@andreasnoack
Copy link
Member

I think we should just merge the lapack wrapper part of this PR. The wrappers will use slightly more memory for now but it still looked like it gave a speedup. Probably because the wrapper then interacts less with the GC. We might be able to make resize! later, but that can be done in a separate PR.

@alyst
Copy link
Contributor Author

alyst commented May 23, 2017

@andreasnoack I've moved the last commit into the new issue for discussions.

alyst added 2 commits June 1, 2017 17:37
- replace a = Vector{T}(n) with resize!(a, n)
- add comments when BLAS API method is called twice
@alyst alyst force-pushed the lapack_use_resize branch from eca256e to 56b6a07 Compare June 1, 2017 15:37
@alyst
Copy link
Contributor Author

alyst commented Jun 1, 2017

#22038 is merged, so this one should not introduce regressions when merged into master.

@ararslan
Copy link
Member

ararslan commented Jun 1, 2017

@nanosoldier runbenchmarks(ALL, vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@KristofferC KristofferC merged commit f1878c1 into JuliaLang:master Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

linear algebra Linear algebra performance Must go faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants