use CheckedSizeProduct.jl for implementing checked_dims#20
use CheckedSizeProduct.jl for implementing checked_dims#20nsajko wants to merge 3 commits intoLilithHafner:mainfrom
checked_dims#20Conversation
|
The package is not registered yet, so this will fail CI, marking as draft. Submitted the PR to get possible input on CheckedSizeProduct before registration. |
This comment was marked as outdated.
This comment was marked as outdated.
|
I'd be willing to give feedback on CheckedSizeProduct.jl. I appreciate your effort making a package for this. I also request that you take more time to verify claims you make when opening PRs to this package. Though I do realize it is harder to test this PR given that CheckedSizeProduct isn't registered.
AFAICT It would be equally easy as it is right now. The flags
julia> malloc(UInt8, typemax(Int))
ERROR: ArgumentError: invalid malloc dimensions
Stacktrace:
[1] #checked_dims#4
@ ~/.julia/packages/PtrArrays/UrKqT/src/PtrArrays.jl:47 [inlined]
[2] checked_dims
@ ~/.julia/packages/PtrArrays/UrKqT/src/PtrArrays.jl:34 [inlined]
[3] malloc(::Type{UInt8}, dims::Int64)
@ PtrArrays ~/.julia/packages/PtrArrays/UrKqT/src/PtrArrays.jl:63
[4] top-level scope
@ REPL[29]:1
This PR does not change the effects of julia> PtrArrays.CheckedSizeProduct
ERROR: UndefVarError: `CheckedSizeProduct` not defined in `PtrArrays`
Suggestion: check for spelling errors or missing imports.
Hint: a global variable of this name also exists in CheckedSizeProduct.
Stacktrace:
[1] getproperty(x::Module, f::Symbol)
@ Base ./Base.jl:42
[2] top-level scope
@ REPL[30]:1
julia> Base.infer_effects(PtrArrays.checked_dims, Tuple{Int, Int})
(+c,+e,!n,+t,+s,+m,+u,+o,+r)
julia> Base.infer_effects(malloc, Tuple{Type{Int}, Int, Int})
(!c,!e,!n,!t,!s,!m,!u,!o,!r)julia> PtrArrays.CheckedSizeProduct
CheckedSizeProduct
julia> Base.infer_effects(PtrArrays.checked_dims, Tuple{Int, Int})
(+c,+e,!n,+t,+s,+m,+u,+o,+r)
julia> Base.infer_effects(malloc, Tuple{Type{Int}, Int, Int})
(!c,!e,!n,!t,!s,!m,!u,!o,!r) |
You're absolutely right about the flags having the same meaning, and I failed to interpret your
I'm sorry, I misinterpreted your implementation and failed to check my assumptions in the REPL.
You forgot about the mandatory keyword argument to On Julia v1.11.2 and PtrArrays julia> InteractiveUtils.@infer_effects PtrArrays.checked_dims(0, 0, 0, 0; message = :malloc)
(+c,!e,!n,!t,!s,!m,!u,!o,!r)On Julia v1.11.2 and PR branch: julia> InteractiveUtils.@infer_effects PtrArrays.checked_dims(0, 0, 0, 0; message = :malloc)
(+c,+e,!n,+t,!s,!m,+u,+o,+r)The result is that, e.g., using Cthulhu, PtrArrays
function func()
malloc(Int, 2, 2, 2)
end
descend(func, Tuple{}) # use the `code_typed` mode, accessible by pressing `T`I know this is a small benefit, but I wanted to get it right given that the package is so small anyway. |
|
You're right about the effects, thanks! Another way to see the impact on effects is julia> @b free(malloc(Int, 2, 2, 2))
577.381 ns (8 allocs: 224 bytes) # main
7.937 ns # prFWIW, the impact is only for >=3 dimensions and is due to JuliaLang/julia#57097 |
You can add a step before PtrArrays.jl/.github/workflows/CI.yml Line 47 in ef99c9f which does import Pkg
Pkg.add(; url="https://github.com/JuliaArrays/CheckedSizeProduct.jl")and then you can actually run CI until the package is registered. |
Benefits: * Better effects. * Prevent the same logic from being reimplemented several times across the ecosystem.
|
CheckedSizeProduct is now simplified and optimized and waiting in the process of registration. |
Benefits:
Better effects.
Prevent the same logic from being reimplemented several times across the ecosystem.