Skip to content

Conversation

ChrisRackauckas
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jun 6, 2022

Codecov Report

Merging #300 (19bfb47) into master (b922313) will decrease coverage by 0.26%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##           master     #300      +/-   ##
==========================================
- Coverage   91.74%   91.47%   -0.27%     
==========================================
  Files           9        9              
  Lines        1429     1420       -9     
==========================================
- Hits         1311     1299      -12     
- Misses        118      121       +3     
Impacted Files Coverage Δ
src/stridelayout.jl 90.53% <88.88%> (ø)
src/axes.jl 92.76% <100.00%> (-0.66%) ⬇️
src/dimensions.jl 96.49% <100.00%> (-0.18%) ⬇️
src/indexing.jl 88.98% <100.00%> (ø)
src/ranges.jl 92.46% <100.00%> (-1.01%) ⬇️
src/size.jl 92.13% <100.00%> (ø)

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 b922313...19bfb47. Read the comment docs.

@Tokazama
Copy link
Member

Tokazama commented Jun 6, 2022

I dropped the == method for Symbol and StaticSymbol because it causes a bunch of invalidations. There's a PR in base to fix that but it hasn't had any movement lately. It would probably be easier for me to just change the tests here.

@Tokazama
Copy link
Member

Tokazama commented Jun 6, 2022

I've fixed the dimension tests. There are still problems with to_shape but I should probably sit down and investigate potential invalidations before defining something there.

@Tokazama
Copy link
Member

Tokazama commented Jun 14, 2022

Now that this is working we can get a good feel for the improvement on load time.

  • master: 0.099550 seconds (127.35 k allocations: 7.828 MiB, 6.37% compilation time)

  • PR: 0.050113 seconds (83.22 k allocations: 5.470 MiB, 10.80% compilation time)

EDIT: This should be even better on Julia nightly because we still have 25 invalidations on v1.8 and some of those have been fixed since.

@Tokazama
Copy link
Member

@chriselrod, what needs to happen downstream for this to work for the JuliaSIMD packages? I'll be available throughout the day to periodically submit PRs if necessary.

@chriselrod
Copy link
Collaborator

@chriselrod, what needs to happen downstream for this to work for the JuliaSIMD packages? I'll be available throughout the day to periodically submit PRs if necessary.

ArrayInterface needs to accept 0.7 before they can accept 0.7.
I think the easiest way to test ArrayInterface + package that depends on Static.jl is to dev them both locally.
Doing so on CI would be difficult, but you could probably create a branch for the other repo where you bump Static compat, and then set the downstream CI here to use that branch.

@chriselrod
Copy link
Collaborator

chriselrod commented Jun 22, 2022

Can we merge this and release?

If we need additional changes, we can make another release, and the dependent packages can set that ArrayInterface version as their lower bound.

@Tokazama
Copy link
Member

I guess we need to rip off the band-aid eventually...

@ChrisRackauckas ChrisRackauckas merged commit f309f48 into master Jun 22, 2022
@ChrisRackauckas ChrisRackauckas deleted the ChrisRackauckas-patch-1 branch June 22, 2022 20:45
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