-
-
Notifications
You must be signed in to change notification settings - Fork 611
Reduce allocations in Dropout #1791
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -30,14 +30,14 @@ The [`Dropout`](@ref) layer is what you should use in most scenarios. | |||
""" | ||||
function dropout(x, p; dims=:, active::Bool=true) | ||||
active || return x | ||||
y = dropout_mask(x, p, dims=dims) | ||||
return x .* y | ||||
y = rand!(similar(x, _dropout_shape(x, dims))) | ||||
x .* _dropout_kernel.(y, p, 1-p) | ||||
end | ||||
|
||||
@adjoint function dropout(x, p; dims=:, active::Bool=true) | ||||
active || return x, Δ -> (Δ, nothing) | ||||
y = dropout_mask(x, p, dims=dims) | ||||
return x .* y, Δ -> (Δ .* y, nothing) | ||||
y = rand!(similar(x, _dropout_shape(x, dims))) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would probably be easier to just remove the call to Edit: a slimmed down Flux.jl/src/layers/normalise.jl Line 114 in 1242c20
|
||||
return x .* _dropout_kernel.(y, p, 1-p), Δ -> (Δ .* _dropout_kernel.(y, p, 1-p), nothing) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me wonder if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That, I believe, would be equivalent to the change here (but perhaps with neater packaging). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe it would also save a multiplication per element (assuming There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it'd be equivalent in the end. Maybe check the generated code to verify. |
||||
end | ||||
|
||||
function dropout_mask(x, p; dims=:) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note BTW that this re-use of
That's another reason to avoid this, in favour of the fusion proposed here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, I think deleting an internal function like this should be fine. If anyone was overloading this for some reason, better they find out sooner than later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this correctly not trigger for GPU arrays? The type of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure this issue exists for GPU, nor whether it calls the method which I pirate here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IDK, but the definition appears specific enough to not cause major problems: https://github.com/JuliaGPU/GPUArrays.jl/blob/master/src/host/broadcast.jl#L50 |
||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allocates a new vector rather than reusing
y
. I tried this variant and it produced the same lowered code as the original.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it's a compelling change, but I don't think it works when dims is set, because then the size of y is actually smaller than x. The current code relies on broadcasting to inflate size-1 dims in the mask to the equivalent full dim size in x.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something here breaks type-stability, which isn't visible on the benchmarks of large arrays:
Also, it is odd that the calculation of
1-p
is pulled out of the kernel, but the more expensive1/q
is not. IMO this should be written_dropout_kernel(y, p, invq) = ifelse(y > p, invq, zero(invq))
, although in examples I tried the compiler does seem to figure this out. But explicit is better than magic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to infer fine for me, perhaps
Random.rand!
wasn't imported beforehand? I get 100.5ns fordropout_pr
and 108.5ns forFlux.dropout
.Riffing of an earlier comment, I wonder if
x
should also be an arg todropout_kernel
. Local benchmarking didn't show much of a difference, but as long as it doesn't hurt codegen it could help to eliminate some redundancy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I can't reproduce this on a restart, not sure what was wrong, sorry.
I doubt it matters much whether you write
x .* _dropout_kernel.(y, p, invq)
or_dropout_kernel.(x, y, p, invq)
, but not opposed. Your hope is roughly that it'll compile onebroadcast
for forwards & back, instead of two?Pulling out the division and avoiding a branch seems like a good idea, although likewise I can't prove it causes issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. It may not work at all, not sure. It's also possible that this should be
y .= rand.(Float32) .> p
.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But confusingly:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does
CUDA.@sync @btime
do? It seems like that would sync once after the benchmark has run, but perhaps it's not like that? I am never sure about GPU benchmarks.The CPU result is sturprising. Note that your
pr_y
is different to mine, it makes a second pass overy
, and broadcasts back to the same array in-place, so it might hit JuliaLang/julia#43153 . I was assuming that, if you materialise an array of random numbers, you should still fuse the.> p
loop with thex
one. These should if anything make it slower, though.In the GPU case, this 2nd pass (over a small array, 50x2?) might mean one more kernel launch, and it's possible this is the entire timing here, 2 vs 1?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
@sync @btime
was just crossed wires on my end, these should be accurate:AFAICT from
@device_code_llvm
, the GPU path doesn't include a similar aliasing check.Edit: another factor to consider is that
rand!
may be quite a bit faster on CPU with 1.7+ because of the new SIMD-friendly Xoshiro implementation.