Fixes OneHotMatrix/Vector GPU Performance #612

DhairyaLGandhi · 2019-02-09T19:21:12Z

Added tests in conjunction with changes made to the behaviour of OneHotVector/Matrix
cc @MikeInnes @KristofferC

MikeInnes · 2019-02-11T13:39:22Z

test/cuda/cuda.jl

@@ -38,6 +38,13 @@ Flux.back!(sum(l))

 end

+@testset "onecold gpu" begin
+  CuArrays.allowscalar(false)


This shouldn't be necessary as part of the test (we call it at the beginning of this file I think).

Right, I'll remove it

MikeInnes · 2019-02-11T13:40:42Z

src/onehot.jl

+function Base.getindex(xs::Flux.OneHotMatrix{T}, ot::Union{Base.Slice, Base.OneTo}, i::Int) where {T<:AbstractArray}
+  res = similar(xs, size(xs, 1), 1)
+  if length(ot) == size(xs, 1)
+    res = xs[:,i]


Why do we need this branch? Are there any cases where they aren't equivalent?

W/O this branch - 136.165 ms (50001 allocations: 1.99 MiB)

julia> A = Flux.onehotbatch(1:300, 1:10000) |> gpu; julia> d = 1 1 julia> a = Base.Slice(axes(A, d)) Base.Slice(Base.OneTo(10000)) julia> A[a, 5] 10000-element Array{Bool,1}: false false false ...

With - 15.930 μs (7 allocations: 10.16 KiB)

julia> A = Flux.onehotbatch(1:300, 1:10000) |> gpu; julia> a = Base.Slice(axes(A, d)) Base.Slice(Base.OneTo(10000)) julia> A[a, 5] 10000-element Flux.OneHotVector: false false false ...

Performance and avoiding the allocation of the vector, basically.

Is that also true on CPU? Is the slowdown due to scalar indexing? It seems like this might need to be something that's fixed at the CuArrays level rather than being special cased here.

I can remove it, if that behaviour is expected and should be maintained.

Scalar indexing happens when we try to get a column out of the .data field from OneHotMatrix currently, which does affect performance.

It's not so much about whether the behaviour is expected as where the bug should be filed. If it can be fixed in CuArrays instead then it should be. It's still not clear to me whether or not that's the case, but I'll take a closer look at the code.

MikeInnes · 2019-02-11T15:12:37Z

src/onehot.jl

@@ -22,6 +24,22 @@ Base.getindex(xs::OneHotMatrix, i::Integer, j::Integer) = xs.data[j][i]
 Base.getindex(xs::OneHotMatrix, ::Colon, i::Integer) = xs.data[i]
 Base.getindex(xs::OneHotMatrix, ::Colon, i::AbstractArray) = OneHotMatrix(xs.height, xs.data[i])

+Base.getindex(xs::Flux.OneHotMatrix, j::Base.UnitRange, i::Int) = xs.data[i][j]


Perhaps just change the above definition to

Base.getindex(xs::OneHotMatrix, i::Union{Integer,AbstractVector}, j::Integer)

MikeInnes · 2019-02-11T15:14:59Z

src/onehot.jl

@@ -22,6 +24,22 @@ Base.getindex(xs::OneHotMatrix, i::Integer, j::Integer) = xs.data[j][i]
 Base.getindex(xs::OneHotMatrix, ::Colon, i::Integer) = xs.data[i]
 Base.getindex(xs::OneHotMatrix, ::Colon, i::AbstractArray) = OneHotMatrix(xs.height, xs.data[i])

+Base.getindex(xs::Flux.OneHotMatrix, j::Base.UnitRange, i::Int) = xs.data[i][j]
+
+Base.getindex(xs::OneHotMatrix, ::Colon, ::Colon) = xs


I think this definition is already handled above. Probably best to leave it; even though it's faster to avoid the allocation, it's more correct to return an independent array.

MikeInnes · 2019-02-11T16:00:06Z

test/onehot.jl

@@ -15,5 +15,4 @@ end
 @testset "onehotbatch indexing" begin
  y = Flux.onehotbatch(ones(3), 1:10)
  @test y[:,1] isa Flux.OneHotVector
-  @test y[:,:] isa Flux.OneHotMatrix


These tests should both still pass, no?

This goes into scalar indexing and allocates the whole array. I had that function to avoid it..

Worth it to have something like getindex(xs::OneHotMatrix, ::Colon, ::Colon) = deepcopy(xs)?

Best to avoid deepcopy -- perhaps copy(xs.data) and reconstruct.

MikeInnes · 2019-02-22T12:47:55Z

My old comment got deleted from the diff, so just to reiterate; there's a line

Base.getindex(xs::OneHotMatrix, i::Integer, j::Integer) = xs.data[j][i]

I think if i::Integer is changed to i::Union{AbstractArray,Integer} then we won't need the extra getindex special cases here.

DhairyaLGandhi · 2019-02-28T04:27:40Z

I am trying to figure out a nice way to do the reduction in the case of an AbstractArray, such that I can avoid allocating intermediaries.mapreducedim! currently returns an LLVM IR error for me, complaining about a call to jl_box_float32.

MikeInnes · 2019-04-04T13:56:43Z

src/onehot.jl

 Base.getindex(xs::OneHotMatrix, ::Colon, i::Integer) = xs.data[i]
 Base.getindex(xs::OneHotMatrix, ::Colon, i::AbstractArray) = OneHotMatrix(xs.height, xs.data[i])
+# Base.getindex(xs::OneHotMatrix, ::Colon, ::Colon) = OneHotMatrix(xs.height, copy(xs.data))


Is this meant to be commented out?

Ah, I found that the second I pushed, fixed with 2952bcd

Thanks, Mike!

bors · 2019-04-05T15:34:43Z

try

Build succeeded

ci/gitlab/trying

MikeInnes · 2019-04-26T10:52:52Z

bors r+

@MikeInnes

612: Fixes OneHotMatrix/Vector GPU Performance r=MikeInnes a=dhairyagandhi96 Added tests in conjunction with changes made to the behaviour of OneHotVector/Matrix cc @MikeInnes @KristofferC Co-authored-by: Dhairya Gandhi <dhairya@juliacopmuting.com>

DhairyaLGandhi · 2019-04-26T11:40:02Z

Hmm.. bors seems to have given up, but the tests have finished https://gitlab.com/JuliaGPU/Flux.jl/pipelines/58592993

MikeInnes · 2019-04-30T12:35:27Z

I guess the internal error is just the usual spurious one? If so feel free to merge.

DhairyaLGandhi · 2019-04-30T13:33:37Z

Those messages indeed are the ones CuArrays has been showing. Will fix the merge conflicts and merge.

Dhairya Gandhi added 2 commits February 9, 2019 22:32

adding tests

35cd976

assert no scalar indexing for onecold

1ada9af

MikeInnes reviewed Feb 11, 2019

View reviewed changes

remove duplicate allowscalar call

d16ef75

MikeInnes reviewed Feb 11, 2019

View reviewed changes

removing non-allocating functions and tests

2ec3586

MikeInnes reviewed Feb 11, 2019

View reviewed changes

mapreduce for onehotmatrix

6825639

KristofferC mentioned this pull request Mar 5, 2019

onecold on Flux.OneHotMatrix{CuArray{Flux.OneHotVector,1}} results in a vector in CPU #660

Closed

KristofferC mentioned this pull request Mar 25, 2019

CuArray broadcast eerror #697

Closed

Dhairya Gandhi added 2 commits April 4, 2019 19:16

fix colon indexing

4f13369

recreate OHV

5b9c534

MikeInnes reviewed Apr 4, 2019

View reviewed changes

fixes

2952bcd

bors bot added a commit that referenced this pull request Apr 5, 2019

Try #612:

5c2e119

Merge branch 'master' into onecold

9bbbd17

DhairyaLGandhi merged commit eff6006 into FluxML:master Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes OneHotMatrix/Vector GPU Performance #612

Fixes OneHotMatrix/Vector GPU Performance #612

DhairyaLGandhi commented Feb 9, 2019

MikeInnes Feb 11, 2019

DhairyaLGandhi Feb 11, 2019

MikeInnes Feb 11, 2019

DhairyaLGandhi Feb 11, 2019

MikeInnes Feb 11, 2019

DhairyaLGandhi Feb 11, 2019

DhairyaLGandhi Feb 11, 2019

MikeInnes Feb 11, 2019

MikeInnes Feb 11, 2019

MikeInnes Feb 11, 2019

MikeInnes Feb 11, 2019

DhairyaLGandhi Feb 11, 2019

DhairyaLGandhi Feb 28, 2019 •

edited

Loading

MikeInnes Apr 4, 2019

MikeInnes commented Feb 22, 2019

DhairyaLGandhi commented Feb 28, 2019

MikeInnes Apr 4, 2019

DhairyaLGandhi Apr 4, 2019

DhairyaLGandhi Apr 4, 2019

bors bot commented Apr 5, 2019

MikeInnes commented Apr 26, 2019

DhairyaLGandhi commented Apr 26, 2019

MikeInnes commented Apr 30, 2019

DhairyaLGandhi commented Apr 30, 2019

Fixes OneHotMatrix/Vector GPU Performance #612

Fixes OneHotMatrix/Vector GPU Performance #612

Conversation

DhairyaLGandhi commented Feb 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhairyaLGandhi Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeInnes commented Feb 22, 2019

DhairyaLGandhi commented Feb 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors bot commented Apr 5, 2019

try

Build succeeded

MikeInnes commented Apr 26, 2019

DhairyaLGandhi commented Apr 26, 2019

MikeInnes commented Apr 30, 2019

DhairyaLGandhi commented Apr 30, 2019

DhairyaLGandhi Feb 28, 2019 •

edited

Loading