Accessors.jl

Accessors.jl copied to clipboard

Because of method invalidations, Requires.jl can cause package load times to be much longer. Accessors currently uses Requires for StaticArrays.jl, which is fairly heavy-weight. But that package has recently been refactored to add a StaticArraysCore.jl, which is very light.

This little PR drops use of Requires in favor of a dependency on StaticArraysCore.

Great I would love to get rid of Requires. The CI fails are StaticArrays related. Do the tests pass locally for you? Does it depend on the StaticArrays version?

Failing for me too. Not sure why, could be a StaticArrays version thing

Locally on master tests pass for me with StaticArrays 1.5.0, StaticArraysCore v1.0.1. Same versions as in CI. So it seems somehow julia generates better code depending on Requires??

StaticArraysCore requires Julia v1.6, as does StaticArrays itself now. Accessors currently still accepts Julia v1.3. I think that's why the tests are failing. Increasing the minimum Julia version to v1.6 in Accessors shouldn't be a problem though, right?

My local tests are still failing on Julia 1.8. Do we need to constrain versions in the test environment? I think that can be done by putting another Project.toml in /test, not sure if there's another way

I just did literally the same Pull Request on Setfield.jl (https://github.com/jw3126/Setfield.jl/pull/174) where tests pass locally (let's see what happens on CI).

If it works there, maybe it helps someone with more insight into the differences between the packages to identify the cause here?

As I said in a comment above, you just removed include("staticarrays.jl"). Add it back, to all other includes.

Also, just curious: is there an actual simple example of Requires making a package load noticeably slower, when only a few lines are within @require? Eg with Setfield.jl or another package.

Requires blocks can invalidate code in other packages, and being at the bottom of a package stack that adds up.

Its really which lines, not how many lines, that matters.

(I'm think the Setfield.jl requires block used to show in the DynamicGrids.jl flamegraph when I was optimising load time, but just a vague memory)

As I said in a comment above, you just removed include("staticarrays.jl"). Add it back, to all other includes.

I don't see any earlier comments from you, but this is a great point. Now the problem is these lines:

@inline delete(obj::StaticArraysCore.SVector, l::IndexLens) = StaticArraysCore.deleteat(obj, only(l.indices))
@inline insert(obj::StaticArraysCore.SVector, l::IndexLens, val) = StaticArraysCore.insert(obj, only(l.indices), val)

deleteat and insert are in StaticArrays, not StaticArraysCore. So this is still broken. I think the solution is to change StaticArraysCore to declare these functions. OTOH that would lead to code invalidation when loading StaticArrays. Any better ideas?

Now the problem is these lines: ... ... change StaticArraysCore to declare these functions.

Yes, we could ask for that - maybe make PRs to StaticArrays and -Core directly. Just declaring the functions in StaticArraysCore would be non-controversial (I would hope).

I don't see any earlier comments from you

Hm, that must be my unfamiliarity with the interface here. Github marks my older comment as "pending", maybe it's only visible to me then? But I don't see any button to actually submit it:

As for Requires overhead: so, unfortunately no simple example anywhere?

Yes, we could ask for that - maybe make PRs to StaticArrays and -Core directly. Just declaring the functions in StaticArraysCore would be non-controversial (I would hope).

Seems to be the case: https://github.com/JuliaArrays/StaticArraysCore.jl/issues/11

Github marks my older comment as "pending", maybe it's only visible to me then? But I don't see any button to actually submit it:

When you add a comment, you can either add it as a one-off or start a review. You must have done the latter. Then you need to click to submit. There should be a button at the top right, "Complete review" or "submit review" or something.

As for Requires overhead: so, unfortunately no simple example anywhere?

If you do @time_imports using MeasureTheory before and after this PR:

Before:

    675.1 ms  Accessors 79.25% compilation time (48% recompilation)

After:

    123.9 ms  Accessors

StaticArraysCore going to be deprecated soon: https://github.com/JuliaArrays/StaticArraysCore.jl/issues/15. The period of relevancy of Core packages wasn't really long :)

So I guess no point in pursuing this further. See https://github.com/JuliaObjects/Accessors.jl/pull/78 for 1.9+ solution.

I close this in favor of #78 Feel free to reopen, if I miss something.