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

loading: provide code for reifying a load-path lookup result #37632

Closed
wants to merge 1 commit into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Sep 17, 2020

This allows asking (and storing) queries about what total set of modules might need to be loaded, before starting any load work. This is base work for later working on #28781, so we might add additional information later (such as folding concrete_deps into it?). This is intended to allow us to pass around the dependency list explicitly, so that worker processes (such as during module loading, precompile, Distributed, and so on) can be isolated from the implicit and mutable LOAD_PATH and Project file parts of the system, once a load is triggered by a toplevel using or cachecompile call.

@vtjnash vtjnash requested a review from KristofferC September 17, 2020 18:35
Copy link
Member

@KristofferC KristofferC left a comment

Choose a reason for hiding this comment

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

This does indeed seem useful to have!

base/loading.jl Outdated
@@ -227,7 +251,7 @@ function locate_package(pkg::PkgId, cache::TOMLCache=TOMLCache())::Union{Nothing
return implicit_manifest_uuid_path(env, pkg, cache)
end
@assert found.uuid !== nothing
return locate_package(found, cache) # restart search now that we know the uuid for pkg
return locate_package(found, cache) # restart search now that we know the uuid for pkg (TODO: the existence of this line of code is probably a bug)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this line feels strange to me too.

@@ -371,6 +425,7 @@ end

# given a directory (implicit env from LOAD_PATH) and a name,
# check if it is an implicit package
# TODO: aren't we supposed to first check for the Project file first and see if it declares a path?
Copy link
Member

Choose a reason for hiding this comment

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

the path entry in Project the project file seems to me like a bit of an experiment that turned out to be unused. Pkg doesn't know about it for example.

uuid === nothing && continue
if UUID(uuid) === where.uuid
found_where = true
# deps is either a list of names (deps = ["DepA", "DepB"]) or
Copy link
Member

Choose a reason for hiding this comment

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

Could some of this code be shared with explicit_manifest_deps_get?

@KristofferC
Copy link
Member

This is intended to allow us to pass around the dependency list explicitly, so that worker processes (such as during module loading, precompile, Distributed, and so on) can be isolated from the implicit and mutable LOAD_PATH and Project file parts of the system, once a load is triggered by a toplevel using or cachecompile call.

In what way did you imagine passing this information to worker. repr feels a bit too slow when large environment data has to be passed (#37989), and we don't really have access to serialization in Base.

@vtjnash
Copy link
Member Author

vtjnash commented Oct 12, 2020

This doesn’t need either form. It’s just a trivial key/value store with a tiny, well-defined set of pairs. We actually already deal with these, just only sending a large subset of them instead of all.

@KristofferC KristofferC force-pushed the jn/loading-plus-plus branch from 8116c50 to 7f66398 Compare May 17, 2021 11:25
@KristofferC
Copy link
Member

Rebased

Allows asking (and storing) queries about what total set of modules might need to be loaded, before starting any load work.
@vtjnash
Copy link
Member Author

vtjnash commented Mar 11, 2022

@KristofferC is this PR still of any interest?

@vtjnash vtjnash closed this Mar 11, 2022
@giordano giordano deleted the jn/loading-plus-plus branch March 31, 2022 20:18
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.

2 participants