meggart / DiskArrays.jl

Other
72 stars 13 forks source link

use indexing rather than broadcast in Array #170

Closed rafaqz closed 3 months ago

rafaqz commented 3 months ago

This MWE is with DiskArrays.jl devved on to latest master

using BenchmarkTools
using ArchGDAL
using Rasters

fn = "deleteme.tif"
x = X(1:4096)
y = Y(1:4096)
bs = 256 # block size
rs = 512 # read size
write(fn, Raster(rand(UInt8, x,y)); options=Dict("TILED"=>"YES", "BLOCKXSIZE"=>bs, "BLOCKYSIZE"=>bs), force=true)

ext = Rasters.Extents.Extent(X=(1, rs), Y=(1, rs))
nt = (min_x=1, min_y=1, max_x=rs, max_y=rs)

r = Raster(fn; lazy=true)
v = Rasters.view(r, ext)

We can time indexing is 3x faster than calling Array. Im not really sure why.

julia> @btime $v[:, :];
  118.211 μs (130 allocations: 261.72 KiB)

julia> @btime Array($v);
  492.226 μs (518 allocations: 533.95 KiB)

This PR just switches Array over to index with all :. so the timings are:

julia> @btime $v[:, :];
@btime Array($v);
  118.788 μs (130 allocations: 261.72 KiB)

julia> @btime Array($v);
  123.338 μs (106 allocations: 261.42 KiB)

(To me this also suggest there is something wrong with our broadcasts... broadcast to a regular array should be almost identical to indexing the whole array, but instead its 3-4x slower)

rafaqz commented 3 months ago

Hmm maybe its better to specialise copyto! for Array instead?

meggart commented 3 months ago

The reason for the speed difference is that broadcast tries to loop over the chunks of the array doing multiple IO steps, while a[:,:] only performs a single IO-operation. I think specializing copyto! for Array is a very good idea, although ideally it should specialize on some ismemory trait. Then we could be sure that the broadcast result is "small" because the user wants to have the result in memory anyway.

Only for broadcast operations where the target is a DiskArray we would need the chunk-by-chunk behavior then.

rafaqz commented 3 months ago

Yes exactly, we should be careful about when we care about chunking. If we know we are trying to move the whole array to memory anyway we should ignore it.

ismemory would be nice to have - it can be forwarded down through the wrappers.

rafaqz commented 3 months ago

Can we merge this as-is for now as a stop-gap solution? and add smarter ismemory detection later?