Accessors.jl icon indicating copy to clipboard operation
Accessors.jl copied to clipboard

Drop Requires, depend on StaticArraysCore

Open cscherrer opened this issue 2 years ago • 14 comments

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.

cscherrer avatar Jul 08 '22 13:07 cscherrer

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?

jw3126 avatar Jul 08 '22 14:07 jw3126

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

cscherrer avatar Jul 08 '22 14:07 cscherrer

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??

jw3126 avatar Jul 08 '22 14:07 jw3126

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?

oschulz avatar Jul 08 '22 14:07 oschulz

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

cscherrer avatar Jul 08 '22 17:07 cscherrer

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?

thomvet avatar Aug 05 '22 12:08 thomvet

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

aplavin avatar Sep 12 '22 19:09 aplavin

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.

aplavin avatar Sep 12 '22 19:09 aplavin

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)

rafaqz avatar Sep 12 '22 20:09 rafaqz

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?

cscherrer avatar Sep 12 '22 20:09 cscherrer

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).

oschulz avatar Sep 12 '22 20:09 oschulz

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: image

aplavin avatar Sep 13 '22 08:09 aplavin

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

aplavin avatar Sep 13 '22 08:09 aplavin

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

cscherrer avatar Sep 13 '22 13:09 cscherrer

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.

aplavin avatar Jan 26 '23 06:01 aplavin

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

jw3126 avatar Jan 26 '23 19:01 jw3126