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

improve cache loading behaviors #45861

Merged
merged 4 commits into from
Jul 4, 2022
Merged

improve cache loading behaviors #45861

merged 4 commits into from
Jul 4, 2022

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jun 29, 2022

@vtjnash vtjnash added the backport 1.8 Change should be backported to release-1.8 label Jun 29, 2022
@KristofferC
Copy link
Member

So to be explicit, what's the status with #45704 after this? Does it now pass tests or give a more descriptive error etc?

@vtjnash
Copy link
Member Author

vtjnash commented Jun 30, 2022

This fixed the iteration order for loaded_modules_array(), so it works now

vtjnash added 4 commits June 30, 2022 13:23
This is theoretically okay, but unlikely to be intended ever.
Does not explicitly close issue #45704, as perhaps the deserialized
module should still be valid after the replacement warning.
Ensures we do not get easily wedged into bad states.
@KristofferC
Copy link
Member

I've tried this on 1.8 and it does indeed resolve #45704

@KristofferC KristofferC merged commit dd5760d into master Jul 4, 2022
@KristofferC KristofferC deleted the jn/45704 branch July 4, 2022 14:30
@KristofferC KristofferC mentioned this pull request Jul 5, 2022
36 tasks
end
@assert found.uuid !== nothing
return locate_package(found) # restart search now that we know the uuid for pkg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can locate_package(found) be added back in? As is, this change results in Base.locate_package(Base.PkgId("Pkg")) returning nothing:

~/julia/julia --startup-file=no
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.9.0-DEV.893 (2022-07-05)
 _/ |\__'_|_|_|\__'_|  |  Commit 8a776bda4c (0 days old master)
|__/                   |

julia> Base.locate_package(Base.PkgId("Pkg"))

julia>

Vs.

❯ julia --startup-file=no
               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.7.3 (2022-05-06)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> Base.locate_package(Base.PkgId("Pkg"))
"/home/awadell/.local/opt/spack/opt/spack/linux-opensuse_leap15-zen3/gcc-11.2.0/julia-1.7.3-fkmbucihyf3jb3jx7plzg6eoqglmh7uz/share/julia/stdlib/v1.7/Pkg/src/Pkg.jl"

julia>

Or is the correct move to use Base.locate_package(Base.identify_package("Pkg"))?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither is "correct" In that both are ambiguous about what they are supposed to return. There is find_package("Pkg") for "internal-Pkg-use" (according to the comment), and other places are usually expected to load a Manifest.toml and locate packages by their [deps] section

awadell1 added a commit to awadell1/PkgJogger.jl that referenced this pull request Jul 5, 2022
Also generally clean up / add more asserts to the `benchmark_dir` code

See: JuliaLang/julia#45861 (comment)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Jul 6, 2022
@IanButterworth
Copy link
Member

This appears to have removed the nesting expected in @time_imports.

@vtjnash is that real? Has nested loading been removed? or has the report printing regressed?

julia> @time_imports using CSV
     50.7 ms  Parsers 17.52% compilation time
      0.2 ms  DataValueInterfaces
      1.6 ms  DataAPI
      0.1 ms  IteratorInterfaceExtensions
      0.1 ms  TableTraits
     17.5 ms  Tables
     26.8 ms  PooledArrays
    193.7 ms  SentinelArrays 75.12% compilation time
      8.6 ms  InlineStrings
     20.3 ms  WeakRefStrings
      2.0 ms  TranscodingStreams
      1.4 ms  Zlib_jll
      1.8 ms  CodecZlib
      0.8 ms  Compat
     13.1 ms  FilePathsBase 28.39% compilation time
   1681.2 ms  CSV 92.40% compilation time

Previously we saw something like:

julia> @time_imports using CSV
      0.4 ms    ┌ IteratorInterfaceExtensions
     11.1 ms  ┌ TableTraits 84.88% compilation time
    145.4 ms  ┌ SentinelArrays 66.73% compilation time
     42.3 ms  ┌ Parsers 19.66% compilation time
      4.1 ms  ┌ Compat
      8.2 ms  ┌ OrderedCollections
      1.4 ms    ┌ Zlib_jll
      2.3 ms    ┌ TranscodingStreams
      6.1 ms  ┌ CodecZlib
      0.3 ms  ┌ DataValueInterfaces
     15.2 ms  ┌ FilePathsBase 30.06% compilation time
      9.3 ms    ┌ InlineStrings
      1.5 ms    ┌ DataAPI
     31.4 ms  ┌ WeakRefStrings
     14.8 ms  ┌ Tables
     24.2 ms  ┌ PooledArrays
   2002.4 ms  CSV 83.49% compilation time

@vtjnash
Copy link
Member Author

vtjnash commented Jul 16, 2022

Nested loading was a bug and is gone now

@tfiers
Copy link
Contributor

tfiers commented Jan 12, 2023

@IanButterworth I am unclear on what the individual @time_imports numbers mean now.
Are they load time for the listed package only; or does it include time to load dependencies ('cumulative') ?

EDIT my bad, saw the comment over at #46072

The times shown are purely the import time of each package now (not the sum of all nested times), given that its deps are loaded further up the order.

Might be worth adding to the docstring?

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