Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Reintroduce Requires.jl since it seems to be faster now #52

Merged
merged 1 commit into from
Mar 27, 2021

Conversation

rofinn
Copy link
Owner

@rofinn rofinn commented Feb 21, 2021

AFAICT, some of the performance issues with using Requires.jl are less relevant on Julia 1.5? Since we've already separated out the FilePathsBase.jl logic into a separate package this seems like a safer way to move forward.

Before re-introducing Requires.jl:

julia> @time using FilePaths
  0.161193 seconds (245.47 k allocations: 16.156 MiB)

julia> @time using FilePaths
  0.774819 seconds (1.74 M allocations: 87.113 MiB, 2.66% gc time)

julia> @time using FilePaths
  0.000248 seconds (280 allocations: 17.531 KiB)

julia> @time using FilePaths
  0.000251 seconds (280 allocations: 17.531 KiB)

Load times after switching to Requires.jl

julia> @time using FilePaths
  0.282045 seconds (386.24 k allocations: 23.427 MiB)

julia> @time using FilePaths
  0.785730 seconds (1.74 M allocations: 87.117 MiB, 1.33% gc time)

julia> @time using FilePaths
  0.000237 seconds (280 allocations: 17.531 KiB)

julia> @time using FilePaths
  0.000260 seconds (280 allocations: 17.531 KiB)

julia> @time using FilePaths
  0.000276 seconds (280 allocations: 17.531 KiB)

Same seems to go if our optional dep is loaded.

Before:

julia> @time using Glob, FilePaths
  0.188511 seconds (269.56 k allocations: 17.710 MiB)

julia> @time using Glob, FilePaths
  0.788237 seconds (1.74 M allocations: 87.125 MiB, 2.77% gc time)

julia> @time using Glob, FilePaths
  0.000393 seconds (577 allocations: 36.141 KiB)

julia> @time using Glob, FilePaths
  0.000416 seconds (577 allocations: 36.141 KiB)

After:

julia> @time using Glob, FilePaths
  1.192447 seconds (2.02 M allocations: 105.242 MiB, 10.89% gc time)

julia> @time using Glob, FilePaths
  0.085030 seconds (135.14 k allocations: 6.965 MiB)

julia> @time using Glob, FilePaths
  0.000441 seconds (577 allocations: 36.141 KiB)

julia> @time using Glob, FilePaths
  0.000387 seconds (577 allocations: 36.141 KiB)

Original performance issues reported here

@codecov
Copy link

codecov bot commented Feb 21, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@f65b053). Click here to learn what that means.
The diff coverage is 100.00%.

❗ Current head 9df324b differs from pull request most recent head b6f4d6d. Consider uploading reports for the commit b6f4d6d to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             master       #52   +/-   ##
==========================================
  Coverage          ?   100.00%           
==========================================
  Files             ?         4           
  Lines             ?        69           
  Branches          ?         0           
==========================================
  Hits              ?        69           
  Misses            ?         0           
  Partials          ?         0           
Impacted Files Coverage Δ
src/glob.jl 100.00% <ø> (ø)
src/uri.jl 100.00% <ø> (ø)
src/FilePaths.jl 100.00% <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 f65b053...b6f4d6d. Read the comment docs.

@oxinabox
Copy link
Contributor

Don't you need to also time how long it then takes to do using Glob? Etc
I thought the main slowness of Revise was in how much it increased time after loading the conditional.

Also deleting things from the dependency list is giving up on letting the package manager do its job and enforcing SemVer.
So while you might want to do this, I think claiming be this is

a safer way to move forward.

Is odd.
Maybe I misunderstood which thing is safer?
Or do you mean: "we are doing this unsafe thing, but at least we are not doing it to FilePathsBase.jl"

@rofinn
Copy link
Owner Author

rofinn commented Feb 22, 2021

Don't you need to also time how long it then takes to do using Glob? Etc

Yes, but the same seems to apply there as well. I've update my description with those times as well. NOTE: I'm only testing on 1.5.

I thought the main slowness of Revise was in how much it increased time after loading the conditional.

Before it was recommended to put the @require call in the __init__ the primary slow point was constantly re-triggering precompilation. There is a slow down in that the glue code isn't precompiled, but in our case that's quite small for now. I think the work around for that is to make the glue code its own package?

Also deleting things from the dependency list is giving up on letting the package manager do its job and enforcing SemVer.

Agreed, but since the package manager doesn't support optional dependencies I don't see a good solution.

  1. As I try to support more of the ecosystem I don't want to introduce a massive list of required dependencies
  2. I don't want a glue package for every extension that'll likely be 1 or 2 simple method overrides

Maybe I misunderstood which thing is safer?
Or do you mean: "we are doing this unsafe thing, but at least we are not doing it to FilePathsBase.jl"

Yeah, I was thinking of compared to when I tried this with the FilePathsBase.jl code. I think the goal is for this to be a repository of simple glue bits which should be small enough to be embedded in projects if they have strict package dependency requirements.

@rofinn rofinn merged commit be89b6e into master Mar 27, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants