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

Code loading: track chosen cachefile and load the module path #37421

Merged
merged 1 commit into from
Sep 13, 2020

Conversation

timholy
Copy link
Member

@timholy timholy commented Sep 5, 2020

These changes allow Revise to leverage precompiled Base machinery, dropping about 150ms from Revise's extra latency penalty for the first package load. Especially for the upcoming Revise3, that's a pretty large fraction of its total latency (ballpark 750ms for both loading Revise and the penalty for the subsequent package load).

Revise had an mtime hack to try to guess which precompile file was being used, so this should make things more robust too since it keeps track of which one actually got loaded.

@@ -813,13 +813,18 @@ function require(into::Module, mod::Symbol)
return require(uuidkey, cache)
end

const pkgcachefiles = Dict{PkgId,String}()
struct PkgOrigin
Copy link
Member

Choose a reason for hiding this comment

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

<3

@timholy
Copy link
Member Author

timholy commented Sep 7, 2020

An alternative to PkgOrigin would be to move Revise's core data structures (or suitably-modified versions of them) to Base: PkgData and its dependencies FileInfo, PkgFiles, and their dependencies. I'm a bit hesitant to suggest this because (1) it's several types that aren't immediately useful in Base, and (2) some of these struct definitions have recently changed, and if they move into Base then the opportunity for quick-fixes diminishes. OTOH, a major part of Revise's remaining startup time is compiling various Dict and OrderedDict methods that operate on these containers, and moving them to Base would allow another considerable reduction in startup latency.

With Revise3's aggregate latency (time to load package + extra overhead for loading the next package while compiling Revise's internals) approaching 0.75s, my overall sense is that now is not the time to contemplate this, but that perhaps around the 1.7 time frame (once Revise3 has had a chance to stabilize) it might be time to consider this option more seriously. But of course, by that point maybe we'll have better precompilation, in which case the benefits diminish unless we think that a lot of the information might be useful anyway.

Thoughts?

@KristofferC
Copy link
Member

Thoughts?

Maybe a bit overkill to move the Revise stuff into Base. As you say, perhaps reevaluate for 1.7?

These changes allow Revise to leverage precompiled Base machinery,
dropping about 150ms from Revise's extra latency penalty for the first
package load.
@timholy timholy merged commit 667ab89 into master Sep 13, 2020
@timholy timholy deleted the teh/loading_revise branch September 13, 2020 22:48
@vtjnash
Copy link
Member

vtjnash commented Sep 15, 2020

Ah, nice. IIUC, this will allow us to trivially fix pathof and pkgdir to now return the correct answers (#33210)?

@timholy
Copy link
Member Author

timholy commented Sep 15, 2020

Yeah, someone will have to figure out the right place to set those values, but presuming that's fairly straightforward this should indeed make it pretty easy.

@fredrikekre
Copy link
Member

See #37586

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.

4 participants