trixi-framework / PointNeighbors.jl

PointNeighbors.jl: Neighborhood search with fixed search radius in Julia
https://trixi-framework.github.io/PointNeighbors.jl/
MIT License
18 stars 5 forks source link

Should we make the GPU stuff a package extension? #47

Open efaulhaber opened 3 months ago

efaulhaber commented 3 months ago

https://github.com/trixi-framework/PointNeighbors.jl/blob/f407b7d7d8bb88e6e7cfd80ccb3907cbb3255728/src/util.jl#L71-L91 I was thinking about making this a package extension, so that we don't add KernelAbstractions.jl as a dependency. However, then users would get unspecific "scalar indexing" errors when they forget to load KernelAbstractions.jl.

This function dispatches on KernelAbstractions.Backend, so without importing KernelAbstractions, I can't do this:

@inline function parallel_foreach(f, iterator,
                                  x::Union{AbstractGPUArray, KernelAbstractions.Backend})
    error("Load KernelAbstractions.jl")
end

Thoughts? @sloede ?

_Originally posted by @efaulhaber in https://github.com/trixi-framework/PointNeighbors.jl/pull/45#discussion_r1660641673_

sloede commented 3 months ago

It would be good to get some input from @lchristm on this, regarding how he handled it.

IIRC, both KA.jl and GPUArrays(Core).jl are somewhat lightweight and could be included as regular dependencies. This allows you to throw around errors based on types, while the heavyweight functionality (and dependencies) can reside in a package extension.

But please double-check if this is the really correctly remembered on my part, at least with KA.jl I am not 100% sure (I'm fairly certain about GPUArraysCore.jl being super cheap)

efaulhaber commented 3 months ago

GPUArraysCore is a subproject of GPUArrays.jl. It's one file, 200 lines, and Adapt.jl is the only dependency.

efaulhaber commented 3 months ago

As discussed with @sloede, we will keep KernelAbstractions.jl as a dependency for now, as it's not too heavy and the extension is not trivial (as I explained above). We can still think about changing that in the future.

lchristm commented 3 months ago

The .jl files in KA.jl/src have less than 2k lines of code overall (including whitespace, docstrings, comments, ...). KA.jl is a very lightweight interface package which defines the kernel language and provides a simple CPU backend (<200 lines of code). All the heavy lifting is done in other packages that implement their own KA.jl backend, like CUDA.jl.

I don't think it negatively impacts load times enough to warrant creating a package extension. From a UX perspective a package extension would have some obvious drawbacks, as already pointed out in the initial post.

If GPUArrays is only used for dispatching then it could be avoided imo, at least in Trixi.jl I track information about the backend and whether KA.jl or "vanilla Trixi" is used via the container types which are available in every function where it matters and can be easily used for dispatching.

efaulhaber commented 3 months ago

If GPUArrays is only used for dispatching then it could be avoided imo, at least in Trixi.jl I track information about the backend and whether KA.jl or "vanilla Trixi" is used via the container types which are available in every function where it matters and can be easily used for dispatching.

GPUArraysCore.jl is tiny and makes the macro much more flexible, especially when used outside PointNeighbors.jl/TrixiParticles.jl for benchmarking or so.