search

JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.43k stars 5.45k forks source link

Arraypocalypse Now and Then #13157

Closed mbauman closed 7 years ago

mbauman commented 9 years ago

This issue supersedes the 0.4 work towards array nirvana (#7941), and will tracks the issues we aim to complete during 0.5 and beyond — now updated through work on 0.7. This is an umbrella issue and will track specific tasks in other issues. Please feel free to add things that I've missed.

Required underlying technologies

Major 0.5 breaking behavior changes

Major 0.6 breaking behavior changes

Possible future breaking changes

New functionality

Other speculative possibilities

tknopp commented 8 years ago

@davidanthoff: Is returning views instead of copies really such a deal breaker to stay away from julia? One can already use sub to get a view into an array.

While I agree that it is always best to make major changes early, there are simply things that take time and in my perspective the Julia release team has done a pretty amazing job in judging which feature is ready for release inclusion and which is not. 0.3 and 0.4 are pretty stable in my point of view.

hayd commented 8 years ago

I was under the impression that the idea was a rapid 0.5 release precisely because of the Array breaking changes (these should be in a single release) were going to be coming soon. If there coming over the next few months, 0.5 should be pushed back?

@ViralBShah The milestone for this issue is still 0.5 isn't it? It can't be 0.4.x?

andreasnoack commented 8 years ago

A while back, I added this benchmark to track how our array performance was relative to Numpy's (which is faster than we are on this benchmark). The benchmark is an LU factorization with complete pivoting and it deliberately allocates a lot of temprorary arrays by working on vectors instead of looping through the elements of the matrix. The Julia version has a copy and a view version of the benchmark so this also gives an indication of how much speedup we'd get from a change to views by default. On my machine, the results are

➜  arrayalloc git:(master) ✗ make
python lucompletepiv.py
size  100 matrix factorized in  0.010 seconds
size  250 matrix factorized in  0.047 seconds
size  500 matrix factorized in  0.246 seconds
size 1000 matrix factorized in  2.330 seconds

Julia version with copy slices
size  100 matrix factorized in  0.016 seconds
size  250 matrix factorized in  0.093 seconds
size  500 matrix factorized in  0.517 seconds
size 1000 matrix factorized in  4.126 seconds

Julia version with view slices
size  100 matrix factorized in  0.004 seconds
size  250 matrix factorized in  0.078 seconds
size  500 matrix factorized in  0.453 seconds
size 1000 matrix factorized in  3.555 seconds

so using views improves the timings a bit, but not enough to match Numpy.

It's very easy to change a copy based algorithm to view based algorithm and this change could be even easier if we introduce special syntax for views. Therefore, it might not be worth the trouble of changing array indexing to views by default.

ViralBShah commented 8 years ago

This is a great starting point. The next obvious step seems like figuring out what we need to do to make subarrays faster. Perhaps @timholy or @mbauman already know.

timholy commented 8 years ago

We need to have the compiler elide subarray construction altogether. I think the subarray code is already well-suited for that, so it's probably "just" hard work on codegen that's needed.

blakejohnson commented 8 years ago

@andreasnoack the difference with small matrices makes this totally worthwhile for my workloads.

timholy commented 8 years ago

Agreed, and note we're faster than python for small matrices.

Hmm, I wonder if there's something else going on---this may not be a benchmark for subarrays at all. What happens if you replace https://github.com/JuliaLang/julia/blob/6d7a50b880fe2189b1efa34eb47d4dfeb181b674/test/perf/arrayalloc/lucompletepiv.jl#L39 with a call to BLAS.syrk2!?

andreasnoack commented 8 years ago

@blakejohnson My main point is that you are already free to use views and we can make this even simpler with syntax.

@timholy It would be faster, but the point of the benchmark is that it allocates temporaries. The Numpy implementation doesn't use syrk2.

davidanthoff commented 8 years ago

@ViralBShah I had interpreted @timholy's comment as a call for more resources (i.e. manpower) for this issue because the devs that intended to do the work on this have moved on. But maybe I misunderstood, or maybe new people have already volunteered to take over.

davidanthoff commented 8 years ago

@tknopp It is not the lack of views from [] that is keeping people from using julia, it is the expectation of major breaking changes coming 1-2 releases down the line. At least I'm surrounded by people for whom julia would be perfect, but that are simply not willing to use a language where they might have to update their code so that it still runs correctly on the next julia release.

I'm not saying that any design decision should be rushed because of that group, I'd rather see things being sorted out for good and taking a little longer. I mainly just wanted to bump this thread, and see whether anyone is still working on things here :)

davidanthoff commented 8 years ago

@andreasnoack Also, your view version doesn't have e.g. @blakejohnson work on bounds removal incorporated, right? My sense was that one of the major points of the general issue here was that there are known perf issues that need to be resolved to make views faster, but that those haven't been done. If things are still not faster once they are done I would understand that argument to just stick with copies by default. But before that call is made, the perf work discussed at the top of this issue should be finished, right?

hayd commented 8 years ago

simply not willing to use a language where they might have to update their code so that it still runs correctly on the next julia release.

Is that because they wouldn't use any pre-1.0 language, or they've heard specifically there will soon be breaking array changes (after which they'd use julia)?

davidanthoff commented 8 years ago

@hayd I'm not 100% sure, but the array stuff I think is especially scary to people because it affects so much code (although, I guess the APL style indexing that is already on master even more so than the view/copy thing).

simonster commented 8 years ago

Are NumPy and Julia using the same BLAS here? I can get a substantial (>1/3) speedup on the size 1000 matrix case by calling Base.disable_threaded_libs().

tknopp commented 8 years ago

@davidanthoff: If this is a concern than Julia is probably too young for these people. In my point of view the number of changes to the core language over the last 3 years were not that drastic. There are of course pretty stable languages but if I think about C++11 or Python3 there are even big changes in mature languages.

timholy commented 8 years ago

@davidanthoff, yes, it's a call for more people to contribute to this effort---previous contributors are probably not "done" (I'm not), but the list is long and individuals have multiple priorities.

A fair bit of array stuff is not actually that hard to work on, because it's a focused part of julia. Or maybe I'm just biased by having collaborated on it with a bunch of super-smart people :smile:. But I think it's amenable to contributions from a larger community, and waiting until some mysterious "they" do all the work may not be the best strategy if "they" don't have time.

Specifically regarding the bounds-check removal: currently views are quite fast, but they are unsafe because there is no bounds-checking. In order to make them generally usable, we need to add bounds-checking, but until @blakejohnson's work merges we can't do that without destroying performance.

@andreasnoack, can you open a separate issue about your lu benchmark? There's more to discuss, but I think it's distracting from the larger issues here.

kkmann commented 8 years ago

What about allowing fields in abstract types (https://github.com/JuliaLang/julia/issues/4935) anything new in that direction?

KristofferC commented 8 years ago

How about stealing the {} syntax from the planned tuple types and have A{:, 1} create a view while A[:, 1] still creates a copy?

StefanKarpinski commented 8 years ago

@JeffBezanson has suggested A@[:,1] for views and A[:,1] for copies as well.

mdcfrancis commented 8 years ago

Naive question why is A[ :,1] not a view by default and if you want a copy you call 'copy' on the view?

To make this terse in a related usage we overloaded '' eg (A[:, 1] ) - if unary * were in the parser then this becomes the terse A[:,1]. Not advocating strongly for the use of \ but it maps in my mind to the C usage.

StefanKarpinski commented 8 years ago

I'm still in favor. @JeffBezanson had some reservations about that – I'll let him explain.

carnaval commented 8 years ago

There are two main issues with views by default IMO

1) it's a breaking change, one of the worst kind since it introduces aliasing bugs (basically, short of deprecating range indexing for a full release you would have to audit every bit of code that uses it)

2) immutables containing julia references are quite inefficient for now so if people start using views for convenience (i.e. to avoid manually computing indices) they will probably see performance regressions

I admit that (2) is something we should work on anyway but it might be important to do it before widespread view usage. I don't see how (1) could be solved without having a separate syntax.

edit: thinking about it, a less troublesome transition for (1) could be to return read only views for a release, avoiding some aliasing bugs (but not those where you write into the original array), and enable writing through a view later

blakejohnson commented 8 years ago

While I appreciate efforts to minimize breaking changes and/or provide deprecation paths... prior to 1.0 shouldn't we be willing to break some things?

ViralBShah commented 8 years ago

I like the idea of having a new syntax for array views. The current indexing behaviour (that generates lots of temporaries) should get faster as we can do better escape analysis and memory reuse. I personally don't know if I like array views by default, and I like the idea of at least exploring potentially different syntax for views.

In general I am in favour of breaking things before 1.0, but this doesn't appear to be a case where it is clear that the new behaviour is a sure shot win.

To take this at least a couple more steps forward, what are the potential candidates for syntax for array views?

ViralBShah commented 8 years ago

There is something interesting about the A@[:,1] syntax, in that it reads A at the location of the following indices.

mlubin commented 8 years ago

There is something interesting about the A@[:,1] syntax, in that it reads A at the location of the following indices.

Agreed, it seems surprisingly intuitive.

ViralBShah commented 8 years ago

Also, it is further growing on me because is very easy to experiment with views in a code with this syntax.

KristofferC commented 8 years ago

My view (ba-ding!) on A@[:,1] is that it is confusing to introduce the macro symbol @ into a completely different context.

ViralBShah commented 8 years ago

That is what my first reaction was, but I got over it for the reasons I wrote above.

mauro3 commented 8 years ago

But similarly, the {} introduce a type-like thing into a completely different context.

kkmann commented 8 years ago

view(A, :, 1)?

StefanKarpinski commented 8 years ago

@kkmann: views need special syntax so that features like end can work.

kmsquire commented 8 years ago

A@[:,1] has potential. +1 for testing it.

mdcfrancis commented 8 years ago

I'd argue that in most cases you want views and only occasionally do you want copies. There are going to be a plethora of threads about code running slowly to which the answer will be 'litter your code with Array@[...]' Can't array views be immutable by default? Then you would have to explicitly request a mutable view.

StefanKarpinski commented 8 years ago

Immutable views by default is an interesting idea. The A@[...] syntax could be for getting a mutable view and copy(A[...]) would be how to get a non-view slice.

mlubin commented 8 years ago

short of deprecating range indexing for a full release you would have to audit every bit of code that uses it

I don't think we have a choice here. If we go ahead with this breaking change, we have to deprecate range indexing for a full release, maybe with an opt-in to disable the warnings. If you don't then @tkelman will get calls about autonomous vehicles driving off the road because code stopped working as intended.

mauro3 commented 8 years ago

But immutable (read-only) views would turn any potential bug into an error, right? Because reading from views is safe.

mlubin commented 8 years ago

@mauro3, not all potential bugs.

from @carnaval:

edit: thinking about it, a less troublesome transition for (1) could be to return read only views for a release, avoiding some aliasing bugs (but not those where you write into the original array), and enable writing through a view later

tkelman commented 8 years ago

The intended meaning of code changes with julia version, and that kind of upgrade is something that people are hopefully being more cautious about than a Pkg.update.

Views by default still feels like the right long term choice to me. Least risky way to get there might be a transition via read only views by default.

johnmyleswhite commented 8 years ago

+1 to a immutable view transition period followed by mutable view

I totally understand the hesitation to make such a breaking change, but I think that argument depends upon understating another risk that's not being emphasized much: the loss of credibility for Julia as a language if it reverses course on a change that's been planned for several years and publicly announced repeatedly. To me, abandoning this planned change feels like a substantive betrayal of trust about the language's long-term course. I don't think the loss of credibility implied by reversing that decision should be understated.

nalimilan commented 8 years ago

How about an argument/instruction to make all attempts to mutate a view raise an error for debugging purposes? Anyway, people need to go through a porting phase to support a new Julia version: they could run their code once with that flag on to identify problematic cases, and be safe after that.

tknopp commented 8 years ago

I vote for just doing views by default. In my point of view this is absolutely consistent with the current behavior of setindex!

julia> a= zeros(3,3)
3x3 Array{Float64,2}:
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> a[:,1] = 1
1

julia> println(a)
[1.0 0.0 0.0
 1.0 0.0 0.0
 1.0 0.0 0.0]

This behavior is inconsistent to the case where a[:,1] is an R-value and silently copied. I therefore am also against making views immutable (which would not be the same as an immutable type by the way).

Of course all this is a design decision and there are pros and cons. But lets decide this early and not postpone it for yet another release.

kmsquire commented 8 years ago

@johnmyleswhite, I think not providing views by default (but still making them easy to access with something like A@[...]) wouldn't be a betrayal of trust. There may be valid reasons not to make indexing mutable by default.

For example, as we move toward multithreading, views by default would mean that I suddenly have to worry that the data I'm working with might be changed by another thread (even if my view of it is immutable). So I might need to add semaphores or copy the data explicitly, and my program and my reasoning about it suddenly becomes more complicated.

mlubin commented 8 years ago

Don't read my comments as opposing views by default, my main point is that we need to be aware of the what should be expected of the transition.

tknopp commented 8 years ago

Would also be interesting how much code this really breaks. We had large discussions that Int8+Int8 -> Int8 will break lots of code but in the end the change got in and there was no massive breakage.

tknopp commented 8 years ago

@kmsquire: Multithreading is in my opinion totally orthogonal to this feature. Yes there are scenarios where it may make sense to copy data in mutithreaded code so that each thread has its own copy but usually this will be done by partitioning an array and operate on parts of this array. And exactly this will require views. Nobody wants to start a bunch of threads in order to copy around code (which [:,:] currently does) in a "hot" loop.

eschnett commented 8 years ago

Would be good to have a syntax that gives you always a view, and that works identically in both 0.4 and 0.5. Ditto for a syntax that makes a copy (without copying twice). If you're worried about backward compatibility, you can then use this syntax, and avoid the syntax that changes meaning.

Isn't this kind of thing always done when making a breaking change? The discussion here conflates two issues -- what the long-term future state should be, and how to go about making this change.

davidanthoff commented 8 years ago

I don't have a strong view on what the final solution should be, I could easily live with any of the suggestions made here.

The only thing I really care about is that it would be great to have all these breaking array changes in one release, namely 0.5, and not stretch them out over multiple releases. I will have to go over all my array code already to accommodate APL style indexing for 0.5, and it would be a great help if whatever breaking change in terms of views came in the same release, so that I only have to change my array code once. My guess is that a fair number of package devs/users would feel the same way.

simonster commented 8 years ago

I think you could find some bugs where a view changes because you've written into the original array by making indexing return an object that wraps both a copy and a view, and checks that the copy matches the view on getindex (and has no setindex!). But that wouldn't be a sensible default, since it's slower than either copies or views.

But in addition to @carnaval's points above, a third point against views by default is that they would not always be a performance win even if implemented efficiently. Even for cases like indexing a vector with a Vector{Int}, if the array is being traversed enough times it's likely to be faster to create a copy. A view into a sparse matrix with a vector of integers as rows seems difficult to make anywhere near as fast as a copy for most operations. Given that whether a view is preferable to a copy depends on what's being indexed and what's being done with it, I think there should be easy ways to do either.

JeffBezanson commented 8 years ago

Given that whether a view is preferable to a copy depends on what's being indexed and what's being done with it, I think there should be easy ways to do either.

This!! Views should not be considered a definite performance win. Mutable views in particular are a very powerful and elaborate feature. I think it's much too difficult to promise that indexing will always return a reference to the underlying data rather than copying. We can't tell everyone who has ever defined getindex that they must now arrange to return a view.

I bet nobody wants scalar indexing to return a view, nor does it seem likely to be implementable efficiently. However with separate syntax we could have even that, i.e. A@[1,1] gives a 0-d view that can be written to with v[] = x.