[ParseableInterface] Make the module caches relocatable and respect -track-system-dependencies #23665

nathawes · 2019-03-29T18:54:17Z

Honor -track-system-dependencies when building parseable interfaces on demand
- FIXME: won't be tracked when the top-level invocation isn't tracking dependencies at all
Honor -track-system-dependencies with -build-module-from-parseable-interface
Serialize dependencies in the SDK as SDK-relative paths
- These are expanded to full paths by appending the SDK path when writing out forwarding modules and when checking dependencies are up-to-date.
Stop serializing .swiftmodule dependencies that are from the regular and prebuilt module caches
- Note: .swiftmodules in the regular module cache still get reported to the dependency tracker. Is this what we want?

Resolves rdar://problem/49292914

nathawes · 2019-03-29T18:54:53Z

jrose-apple

I did a quick read and it looks pretty good. @harlanhaskins should probably take a look too, and I'll get back to it a second time later today.

test/ParseableInterface/ModuleCache/SDKDependencies.swiftinterface

lib/Frontend/ParseableInterfaceModuleLoader.cpp

test/ParseableInterface/ModuleCache/SDKDependencies.swift

jrose-apple · 2019-03-29T20:58:47Z

Note: .swiftmodules in the regular module cache still get reported to the dependency tracker. Is this what we want?

I've come to a conclusion on this: if we compile the prebuilt modules out of order, they will load swiftmodules from the regular module cache, and it would be bad to mark those as dependencies.

lib/Frontend/ParseableInterfaceModuleLoader.cpp

harlanhaskins

Small comment, otherwise this looks great!

nathawes · 2019-03-30T02:09:37Z

Note: .swiftmodules in the regular module cache still get reported to the dependency tracker. Is this what we want?

I've come to a conclusion on this: if we compile the prebuilt modules out of order, they will load swiftmodules from the regular module cache, and it would be bad to mark those as dependencies.

Good point! I've updated the SDKDependencies.swift test to build the prebuilt modules in both orders and check they don't have any FILE_DEPENDENCY entries containing the module cache path in either case.

jrose-apple

Okay, went through it again and I think it's good! Minor comments only.

lib/Frontend/ParseableInterfaceModuleLoader.cpp

test/ParseableInterface/ModuleCache/SDKDependencies.swift

nathawes · 2019-04-01T19:08:23Z

@swift-ci please test

swift-ci · 2019-04-01T19:10:32Z

Build failed
Swift Test Linux Platform
Git Sha - 03716cb33f7d5085da5ca02ba0554571c3582db9

nathawes · 2019-04-01T19:13:58Z

@swift-ci please test OS X Platform

nathawes · 2019-04-01T20:43:09Z

@swift-ci please test OS X Platform

nathawes · 2019-04-01T21:24:45Z

@swift-ci please smoke test OS X Platform

swift-ci · 2019-04-01T21:45:09Z

Build failed
Swift Test OS X Platform
Git Sha - 03716cb33f7d5085da5ca02ba0554571c3582db9

nathawes · 2019-04-02T19:56:32Z

@swift-ci please test

swift-ci · 2019-04-02T19:58:30Z

Build failed
Swift Test OS X Platform
Git Sha - 1e05cff26c2e516f09b7dc586bebae094a83b194

swift-ci · 2019-04-02T19:58:31Z

Build failed
Swift Test Linux Platform
Git Sha - 1e05cff26c2e516f09b7dc586bebae094a83b194

nathawes · 2019-04-02T20:03:19Z

lib/Frontend/ParseableInterfaceModuleLoader.cpp

-      // If Dep is itself a .swiftmodule in the cache dir, pull out its deps
-      // and include them in our own, so we have a single-file view of
-      // transitive deps: removes redundancies, and avoids opening and reading
-      // multiple swiftmodules during future loads.


It seems like pulling these in was redundant? I think everything still works because whenever we load/depend on something from the cache we end up adding its serialized dependencies to the dependency tracker when validating that it's up to date, so they show up as one of the initial dependencies here and get serialized.

That doesn't sound right. What if it's out of date and then we rebuild it and discover it doesn't depend on some of those files anymore?

ParseableInterfaceModuleLoaderImpl::dependenciesAreUpToDate() is called by both forwardingModuleIsUpToDate() and swiftModuleIsUpDate() and adds the dependencies to the tracker regardless of whether they are out of date or not, so looks like that's a bug. I'll add a test for it and update the code to only add them to the tracker if they're all up to date.

In the case that they're not up to date though, we then go down the path of generating the swiftmodule from the interface again, and that adds all the SubInstance's dependencies to the tracker (in collectDepsForSerialization()). Since all of its dependencies must be either up to date or rebuilt too, I think us pulling out the serialized deps here is redundant regardless. Does that make sense, or am I still missing something?

I'm surprised that dependenciesAreUpToDate adds dependencies to the outer tracker at all. That doesn't seem like its job, and that would explain why this code is here. But having it do that does save us a bit of effort in reading from the cached compiled module twice. It at least deserves to be commented loudly, though, since that name sounds like a pure predicate function.

Yeah, dependenciesAreUpToDate should probably be renamed here. Good catch noticing that it doesn't check if they actually are up to date 😳

I ended up making it not add the dependencies and added them a few levels up in findOrBuildLoadableModule and updated it's doc to mention that it adds the dependencies to the dependencyTracker too.

nathawes · 2019-04-02T23:27:41Z

@swift-ci please test

swift-ci · 2019-04-02T23:29:45Z

Build failed
Swift Test Linux Platform
Git Sha - 934c8ff60150ca848be323e063884fbf10244bb0

nathawes · 2019-04-03T00:43:25Z

@swift-ci please test

swift-ci · 2019-04-03T00:45:43Z

Build failed
Swift Test Linux Platform
Git Sha - ff30b30b31378e364e4d1bad47d913cd3967b537

swift-ci · 2019-04-03T01:11:38Z

Build failed
Swift Test OS X Platform
Git Sha - 934c8ff60150ca848be323e063884fbf10244bb0

jrose-apple

Looks good to me, only small comments left.

include/swift/Serialization/SerializedModuleLoader.h

test/ParseableInterface/ModuleCache/module-cache-init.swift

nathawes · 2019-04-03T03:36:35Z

@swift-ci please test

swift-ci · 2019-04-03T03:38:37Z

Build failed
Swift Test Linux Platform
Git Sha - 0227614bedc0979254f9523ce270717333890e9c

swift-ci · 2019-04-03T04:32:47Z

Build failed
Swift Test OS X Platform
Git Sha - 0227614bedc0979254f9523ce270717333890e9c

One interesting thing here is that the decision of whether or not to track system dependencies ends up going into the cache key for a swiftmodule built from a parseable interface, because it affects that module's up-to-date check. If we didn't include -track-system-dependencies in the cache key, we could end up tracking system dependencies for some modules but not others in the same build. There's a bit of a bug here where they're /not/ honored if the top-level invocation isn't tracking /any/ dependencies, but given how uncommon this flag is in practice that's probably okay for now. Still TODO: honor this for -build-swiftmodule-from-parseable-interface as well. That's currently not tracking dependencies at all and it probably should.

…odule-from-parseable-interface Updates the subinvocation that builds the parseable interface to respect the -track-system-dependencies flag of the top-level invocation if present, by including system dependencies in the produced .swiftmodule.

This allows the SDK to be relocated without automatically resulting in a rebuild. Based on an old patch from Jordan Rose.

…module cache or prebuilt module cache *Their* dependencies are already being serialized out, so this shouldn't affect up-to-date-checking except by alowing the regular and prebuilt module caches to be relocated without invalidating their contents. In the case of the prebuilt module cache, this gets us closer to supporting relocation across machines.

…citly check serialized dependencies Also use the existing check-is-forwarding-module.py script to check for forwarding modules, rather than doing it manually.

… adding cached modules to the dependency tracker This patch modifies ParseableInterfaceBuilder::CollectDepsForSerialization to avoid serializing dependencies from the runtime resource path into the swiftmodules generated from .swiftinterface files. This means the module cache should now be relocatable across machines. It also modifies ParseableInterfaceModuleLoader to never add any dependencies from the module cache and prebuilt cache to the dependency tracker (in addition to the existing behaviour of not serializing them in the generated swiftmodules). As a result, CollectDepsForSerialization no longer checks if the dependencies it is given come from the cache as they are provided by the dependency tracker. It now asserts that's the case instead.

…ent dependency tracker We previously added dependencies to the tracker inline while validating a cached module's dependencies were up to date. If one of its dependencies ended up being out of date though, we shouldn't have added the previous dependencies, as that means the dependency list itself was also out of date. This patch changes the behavior to only add the module's dependencies once we've verified they're all up to date.

nathawes · 2019-04-03T13:35:35Z

@swift-ci please smoke test

compnerd · 2019-04-03T17:54:46Z

This caused a regression in the Windows build! https://dev.azure.com/compnerd/windows-swift/_build/results?buildId=971

nathawes · 2019-04-03T20:08:42Z

Sorry @compnerd! PR to hopefully fix the two parseable interface test failures here: #23773

nathawes requested review from jrose-apple and harlanhaskins and removed request for jrose-apple March 29, 2019 18:54

jrose-apple reviewed Mar 29, 2019

View reviewed changes

harlanhaskins reviewed Mar 29, 2019

View reviewed changes

lib/Frontend/ParseableInterfaceModuleLoader.cpp Outdated Show resolved Hide resolved

harlanhaskins approved these changes Mar 29, 2019

View reviewed changes

jrose-apple mentioned this pull request Mar 29, 2019

[ParseableInterface] Distinguish SDK and non-SDK dependencies #22733

Closed

nathawes force-pushed the it-all-depends-take-2 branch from 03716cb to 491d11c Compare March 30, 2019 01:42

jrose-apple reviewed Apr 1, 2019

View reviewed changes

lib/Frontend/ParseableInterfaceModuleLoader.cpp Outdated Show resolved Hide resolved

test/ParseableInterface/ModuleCache/SDKDependencies.swift Outdated Show resolved Hide resolved

test/ParseableInterface/ModuleCache/SDKDependencies.swift Show resolved Hide resolved

nathawes force-pushed the it-all-depends-take-2 branch 4 times, most recently from d52c19c to 1e05cff Compare April 1, 2019 19:08

nathawes requested review from harlanhaskins and jrose-apple April 2, 2019 19:57

nathawes commented Apr 2, 2019

View reviewed changes

nathawes force-pushed the it-all-depends-take-2 branch from ff30b30 to 0227614 Compare April 3, 2019 00:43

nathawes mentioned this pull request Apr 3, 2019

[5.1][ParseableInterface] Make the module caches relocatable and respect -track-system-dependencies #23756

Merged

jrose-apple reviewed Apr 3, 2019

View reviewed changes

include/swift/Serialization/SerializedModuleLoader.h Outdated Show resolved Hide resolved

test/ParseableInterface/ModuleCache/module-cache-init.swift Outdated Show resolved Hide resolved

nathawes force-pushed the it-all-depends-take-2 branch from 0227614 to 46e5c86 Compare April 3, 2019 03:36

jrose-apple and others added 7 commits April 3, 2019 06:34

[ParseableInterface] Distinguish SDK and non-SDK dependencies

58d0ee0

This allows the SDK to be relocated without automatically resulting in a rebuild. Based on an old patch from Jordan Rose.

[ParseableInterface][test] Update SDKDependencies.swift test to expli…

accc647

…citly check serialized dependencies Also use the existing check-is-forwarding-module.py script to check for forwarding modules, rather than doing it manually.

nathawes force-pushed the it-all-depends-take-2 branch from 46e5c86 to 7144dab Compare April 3, 2019 13:35

nathawes merged commit b8a1f2e into swiftlang:master Apr 3, 2019

nathawes deleted the it-all-depends-take-2 branch April 3, 2019 15:02

[ParseableInterface] Make the module caches relocatable and respect -track-system-dependencies #23665

[ParseableInterface] Make the module caches relocatable and respect -track-system-dependencies #23665

Uh oh!

Conversation

nathawes commented Mar 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nathawes commented Mar 29, 2019

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jrose-apple commented Mar 29, 2019

Uh oh!

Uh oh!

harlanhaskins left a comment

Choose a reason for hiding this comment

Uh oh!

nathawes commented Mar 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nathawes commented Apr 1, 2019

Uh oh!

swift-ci commented Apr 1, 2019

Uh oh!

nathawes commented Apr 1, 2019

Uh oh!

nathawes commented Apr 1, 2019

Uh oh!

nathawes commented Apr 1, 2019

Uh oh!

swift-ci commented Apr 1, 2019

Uh oh!

nathawes commented Apr 2, 2019

Uh oh!

swift-ci commented Apr 2, 2019

Uh oh!

swift-ci commented Apr 2, 2019

Uh oh!

nathawes Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

jrose-apple Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

nathawes Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

jrose-apple Apr 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harlanhaskins Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

nathawes Apr 2, 2019

Choose a reason for hiding this comment

Uh oh!

nathawes commented Apr 2, 2019

Uh oh!

swift-ci commented Apr 2, 2019

Uh oh!

nathawes commented Apr 3, 2019

Uh oh!

swift-ci commented Apr 3, 2019

Uh oh!

swift-ci commented Apr 3, 2019

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

nathawes commented Mar 29, 2019 •

edited

Loading

nathawes commented Mar 30, 2019 •

edited

Loading

jrose-apple Apr 2, 2019 •

edited

Loading