Skip to content

Conversation

chriselrod
Copy link
Collaborator

@ffreyer you can try this

@codecov
Copy link

codecov bot commented Sep 2, 2022

Codecov Report

Merging #343 (c42c20b) into master (bf07fa9) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #343      +/-   ##
==========================================
+ Coverage   90.06%   90.23%   +0.16%     
==========================================
  Files           9        9              
  Lines        1319     1331      +12     
==========================================
+ Hits         1188     1201      +13     
+ Misses        131      130       -1     
Impacted Files Coverage Δ
src/array_index.jl 100.00% <100.00%> (ø)
src/dimensions.jl 93.61% <0.00%> (+0.04%) ⬆️
src/stridelayout.jl 89.89% <0.00%> (+0.38%) ⬆️
src/ArrayInterface.jl 85.96% <0.00%> (+0.64%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@ffreyer
Copy link

ffreyer commented Sep 2, 2022

Doesn't seem to help

julia> @benchmark reflectorApply!($x, $τ, $y)
BenchmarkTools.Trial: 10000 samples with 1 evaluation.
 Range (min  max):  46.028 μs  210.703 μs  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     49.761 μs               ┊ GC (median):    0.00%
 Time  (mean ± σ):   52.597 μs ±  11.745 μs  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▇▇███▇▅▃▁    ▁▂▃▃▃▃▂          ▂▂▁                            ▃
  ███████████▆▅███████▇▅▆▆▅▄█▇█████▇▅▅▅▅▅▅▆▇▇▇█▆▆▆▃▄▃▁▁▃▄▆▃▆▆▅ █
  46 μs         Histogram: log(frequency) by time       105 μs <

 Memory estimate: 0 bytes, allocs estimate: 0.

(@v1.7) pkg> st ArrayInterface
      Status `~/.julia/environments/v1.7/Project.toml`
  [4fba245c] ArrayInterface v6.0.22 `https://github.com/JuliaArrays/ArrayInterface.jl.git#inlinestrideindex`

@Tokazama
Copy link
Member

Tokazama commented Sep 2, 2022

We might need a more aggressive fix for for previous versions

function map_tuple_type(f::F, ::Type{T}) where {F,T<:Tuple}
    if @generated
        t = Expr(:tuple)
        for i in 1:fieldcount(T)
            push!(t.args, :(f($(fieldtype(T, i)))))
        end
        Expr(:block, Expr(:meta, :inline), t)
    else
        Tuple(f(fieldtype(T, i)) for i in 1:fieldcount(T))
    end
end

@chriselrod
Copy link
Collaborator Author

We might need a more aggressive fix for for previous versions

You're welcome to take over this PR, or we can merge it and you follow up with another?

I was waiting on integration tests to merge, but this PR seems like it should be harmless w/ respect to semantics.

@Tokazama
Copy link
Member

Tokazama commented Sep 2, 2022

We might need a more aggressive fix for for previous versions

You're welcome to take over this PR, or we can merge it and you follow up with another?

I was waiting on integration tests to merge, but this PR seems like it should be harmless w/ respect to semantics.

Sounds good. I put in the old generated methods. I'll wait for some tests to pass before merging but your probably right that we don't need to wait for all of the integration tests.

@Tokazama Tokazama merged commit 951f2a4 into master Sep 2, 2022
@Tokazama Tokazama deleted the inlinestrideindex branch September 2, 2022 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants