[ParseableInterface] Flesh out parseable module loading #22603
Conversation
test/ParseableInterface/ModuleCache/prebuilt-module-cache-forwarding.swift
Outdated
Show resolved
Hide resolved
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
6 times, most recently
from
2b706ab to
a3a0e01
Compare
harlanhaskins
changed the title
harlanhaskins
changed the title
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
3 times, most recently
from
700990e to
77e604f
Compare
|
@swift-ci please test |
|
Build failed |
|
@swift-ci please test macOS platform |
|
Build failed |
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
from
77e604f to
a738308
Compare
|
@swift-ci please python lint |
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
from
a738308 to
38b10fa
Compare
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
from
38b10fa to
4a049a2
Compare
|
@swift-ci please python lint |
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
2 times, most recently
from
0cc522c to
b37bba0
Compare
|
Build failed |
Add a bit to the module to determine whether the dependency’s stored bit pattern is a hash or an mtime. Prebuilt modules store a hash of their dependencies because we can’t be sure their dependencies will have the same modtime as when they were built.
Since prebuilt modules are going to use hashes for their dependencies, we need a flag to turn that behavior on. By default, we use modification times.
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
2 times, most recently
from
b1d0d17 to
c932ec8
Compare
benlangmuir
left a comment
benlangmuir
left a comment
There was a problem hiding this comment.
Something we do for clang modules that I don't see here: if there are multiple processes trying to load the same module, we use lock files to avoid rebuilding the same module concurrently. This isn't needed for correctness if you can keep the just-built module in memory, but it could save battery. Thoughts?
There was a problem hiding this comment.
Is this a canonicalized path? If not, maybe it should be?
There was a problem hiding this comment.
It's whatever module lookup resolved to, which may have come from command-line search paths. So it probably should be.
There was a problem hiding this comment.
Is this a canonicalized path? If not, maybe it should be?
There was a problem hiding this comment.
Looks like the only canonicalization we perform is removing a trailing '/' when it's set. Given that this is a command-line flag, we probably should.
|
Based on the number of comments you may wish to refactor and resubmit. This many comments does not mean your idea is bad. Simply that it is not ready for review. |
|
@openloop, I don't think there's anything here that requires a new PR. The author has been working closely with the reviewers to solve the outstanding issues. |
benlangmuir
left a comment
benlangmuir
left a comment
There was a problem hiding this comment.
@harlanhaskins will address the issues I raised about re-reading module files and re-stat-ing as follow-ups, and similarly for the optimization level. My other feedback has been resolved. Thanks!
A ‘forwarding module’ is a YAML file that’s meant to stand in for a .swiftmodule file and provide an up-to-date description of its dependencies, always using modification times. When a ‘prebuilt module’ is first loaded, we verify that it’s up-to-date by hashing all of its dependencies. Since this is orders of magnitude slower than reading mtimes, we’ll install a `forwarding module` containing the mtimes of the now-validated dependencies.
harlanhaskins
force-pushed
the
where-do-i-stash-my-cache-hashes
branch
from
616a949 to
366bbf4
Compare
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
apple/swift-lldb#1328 |
|
Build failed |
nathawes
left a comment
nathawes
left a comment
There was a problem hiding this comment.
Looks good to me, Harlan! I only had one trivial comment that I wouldn't worry about unless you have other changes to make anyway.
|
|
||
| bool isNormal() const { return kind == Kind::Normal; } | ||
| bool isPrebuilt() const { return kind == Kind::Prebuilt; } | ||
| bool isForwarding() const { return kind == Kind::Forwarded; } |
There was a problem hiding this comment.
Should this be isForwarded() to match the earlier comments that distinguish the Forwarding module in the regular cache from the Forwarded module in the prebuilt cache?
There was a problem hiding this comment.
Yep, it should. I'll fix that in a follow-up 😅 It's also unused, so maybe this and isNormal can just go.
|
apple/swift-lldb#1328 |
|
Build failed |
|
Build failed |
|
apple/swift-lldb#1328 |
|
LLDB failure doesn't seem related. apple/swift-lldb#1328 |
|
Build failed |
|
apple/swift-lldb#1328 |
…h-my-cache-hashes [ParseableInterface] Flesh out parseable module loading
5 participants
Essentially, this changes how we process the entries in the prebuilt cache dir. Previously, we would always take whatever's in the prebuilt cache, even if its dependencies changed, invalidating it. This emulated the current module loading behavior, where we don't have the option to fall back on recompiling a parseable interface.
With this change, dependencies of prebuilt modules are based on the hash of the contents of the dependency, rather than modification times. Since hashing every dependency is expensive, we install a smaller "forwarding module" in the Regular Cache, which contains the mtimes from the last time we validated the dependencies. On further loads of this module, we will validate dependencies using the forwarding module and then load the underlying module without validating its dependencies.