Skip to content
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

fix code instability of axes #119

Merged
merged 2 commits into from
Jul 2, 2020
Merged

fix code instability of axes #119

merged 2 commits into from
Jul 2, 2020

Conversation

johnnychen94
Copy link
Member

@johnnychen94 johnnychen94 commented Jun 12, 2020

This fixes #116

Switching from Base.OneTo to IdOffsetRange might introduces some incompatibility issue for downstream packages since IIUC IdOffsetRange is still at experimental status, so I'd prefer to not merge this PR until we revisit and polish the design of IdOffsetRange.

P.S., perhaps surprisingly, the type instability in #116 doesn't slow down the codes:

julia> @btime axes(a)
  17.472 ns (1 allocation: 32 bytes)
(-1:1,)

julia> @btime axes(a, 1)
  18.725 ns (1 allocation: 32 bytes)
-1:1

julia> @btime axes(a, 2)
  17.037 ns (0 allocations: 0 bytes)
Base.OneTo(1)

cc: @jishnub

@codecov
Copy link

codecov bot commented Jun 12, 2020

Codecov Report

Merging #119 into master will increase coverage by 4.14%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   83.69%   87.83%   +4.14%     
==========================================
  Files           2        2              
  Lines         184      222      +38     
==========================================
+ Hits          154      195      +41     
+ Misses         30       27       -3     
Impacted Files Coverage Δ
src/OffsetArrays.jl 91.06% <50.00%> (+4.57%) ⬆️
src/axes.jl 74.41% <0.00%> (+2.19%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7294f3d...61d5031. Read the comment docs.

@goretkin
Copy link
Contributor

I don't think this will be an issue in practice, but worth noting that

julia> @code_llvm first(Base.OneTo(3))

;  @ range.jl:566 within `first'
define i64 @julia_first_19563([1 x i64] addrspace(11)* nocapture nonnull readonly dereferenceable(8)) {
top:
  ret i64 1
}

but

julia> @code_llvm first(OffsetArrays.IdOffsetRange(Base.OneTo(3)))

;  @ /Users/goretkin/.julia/dev/OffsetArrays/src/axes.jl:150 within `first'
define i64 @julia_first_19558({ [1 x i64], i64 } addrspace(11)* nocapture nonnull readonly dereferenceable(16)) {
top:
; ┌ @ Base.jl:33 within `getproperty'
   %1 = getelementptr inbounds { [1 x i64], i64 }, { [1 x i64], i64 } addrspace(11)* %0, i64 0, i32 1
; └
; ┌ @ int.jl:53 within `+'
   %2 = load i64, i64 addrspace(11)* %1, align 8
   %3 = add i64 %2, 1
; └
  ret i64 %3
}

The reason I think the type instability here is fine is due to constant propagation, is my guess.

@johnnychen94 johnnychen94 merged commit 320989a into master Jul 2, 2020
@johnnychen94 johnnychen94 deleted the jc/axes branch July 2, 2020 14:54
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.

Type instability in axes(arr,dim)
3 participants