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

Drop Requires, depend on StaticArraysCore #60

Closed
wants to merge 7 commits into from

Conversation

cscherrer
Copy link

@cscherrer cscherrer commented Jul 8, 2022

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.

@jw3126
Copy link
Member

jw3126 commented Jul 8, 2022

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?

@cscherrer
Copy link
Author

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

@jw3126
Copy link
Member

jw3126 commented Jul 8, 2022

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

@oschulz
Copy link

oschulz commented Jul 8, 2022

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?

Project.toml Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
Co-authored-by: Oliver Schulz <[email protected]>
@cscherrer
Copy link
Author

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

@thomvet
Copy link

thomvet commented Aug 5, 2022

I just did literally the same Pull Request on Setfield.jl (jw3126/Setfield.jl#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?

@aplavin
Copy link
Member

aplavin commented Sep 12, 2022

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

@aplavin
Copy link
Member

aplavin commented Sep 12, 2022

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.

@rafaqz
Copy link
Member

rafaqz commented Sep 12, 2022

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)

@cscherrer
Copy link
Author

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?

@oschulz
Copy link

oschulz commented Sep 12, 2022

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

@aplavin
Copy link
Member

aplavin commented Sep 13, 2022

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
Copy link
Member

aplavin commented Sep 13, 2022

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

@cscherrer
Copy link
Author

cscherrer commented Sep 13, 2022

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:
JuliaArrays/StaticArraysCore.jl#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

@aplavin
Copy link
Member

aplavin commented Jan 26, 2023

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

So I guess no point in pursuing this further. See #78 for 1.9+ solution.

@jw3126
Copy link
Member

jw3126 commented Jan 26, 2023

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

@jw3126 jw3126 closed this Jan 26, 2023
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.

6 participants