[Incremental] Optimize incremental build logic by interning strings #800

davidungar · 2021-08-17T21:56:06Z

No description provided.

davidungar · 2021-08-17T21:56:25Z

@swift-ci please test macos platform

davidungar · 2021-08-19T03:14:12Z

@swift-ci please test

davidungar · 2021-08-19T18:58:41Z

@swift-ci please test

davidungar · 2021-08-19T20:23:25Z

@swift-ci please test

davidungar · 2021-08-19T20:25:55Z

rdar://75386359

davidungar · 2021-08-19T21:10:15Z

Relative savings on some test:
29% testCleanBuildSwiftDepsPerformance
25% testSavingPriorsPerformance
9% testReadingPriorsPerformance
However, these won't reflect overall savings.

davidungar · 2021-08-19T21:10:33Z

@swift-ci please test

artemcm · 2021-08-19T21:36:14Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+  let index: Int
+  let table: InternedStringTable
+
+  fileprivate init(_ s: String, host: ModuleDependencyGraph) {


The use of the module dependency graph here is confusing.
Why should a generic interned string have any connection to an incremental-build-related data structure?

I think it should be the String cache entity itself that we rely on here, not where that cache is meant to live (ModuleDependencyGraph).

artemcm · 2021-08-19T21:40:57Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/DependencySource.swift

@@ -55,12 +67,15 @@ extension DependencySource {
  /// Throws if a read error
  /// Returns nil if no dependency info there.
  public func read(
-    info: IncrementalCompilationState.IncrementalDependencyAndInputSetup
+    info: IncrementalCompilationState.IncrementalDependencyAndInputSetup,
+    host: ModuleDependencyGraph


We now often have the use of host throughout that I can't wrap my head around by locally looking at the code. From the rest of this PR I gather that we are passing around the dependency graph because that's where the interned string cache lives, but the name host doesn't tell me that. Perhaps we can pass around a reference to the actual string cache to where it is needed?

artemcm · 2021-08-19T21:48:02Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+}
+
+/// Hardcode empty as 0
+public class InternedStringTable {


This feels very close to our existing PathCache implementation (https://github.com/apple/swift-driver/blob/main/Sources/SwiftDriver/Utilities/VirtualPath.swift#L325).

Is this functionality we can common-out or maybe model this approach after the already-existing one?

Why do we not require synchronization in this case? If this table lives in the module-dependency-graph, can't we be accessing that concurrently after compilation jobs finish?

With filepaths, we have one global cache with synchronized accesses, whereas here we are relying on the specific instance of the string cache/table to be specified whenever we are dealing with interned strings. I wonder why we can't rely on a global cache here as well, much like VirtualPath? That way we won't have to tie this to the dependency graph and plumb the graph to all the use-sites.

Why do we not require synchronization in this case?

In theory, all usages of this class should be as thread safe as the calling context. Integration is currently a serial process in all of the mutating paths, but that's a somewhat ad-hoc thing at the moment in that it's only really being enforced manually by IncrementalCompilationState. In practice, there will be interners on both the main queue and on the private queue maintained by IncrementalCompilationState which could lead to data races on the cache.

What this suggests to me is that we need to make it structurally impossible for anything but serialization and the module dependency graph to mutate these things, which is currently not the case with this API. Where that is not possible, we should be asserting that accesses are occurring on the expected queue (dispatchPrecondition can help).

Yes, the refactor I did before set up IncrementalCompilationState as the serialization point for the graph. Maybe the table should live there instead of the graph, and be protected there?
I worry that dispatchPrecondition on every access could damage performance. How does its overhead compare to a dictionary-keyed-by-string lookup?

artemcm · 2021-08-19T21:48:27Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+  public var isEmpty: Bool { index == 0 }
+
+  // PRIVATE?
+  init(index: Int, table: InternedStringTable) {


Yeah, I don't think we should allow this to be used publicly.

davidungar · 2021-09-09T20:50:30Z

Driver CPU times measured by Instruments, on a 1000+ source-file project, 3 runs each

clean build baseline: 20.65, 20.67, 20.08
clean build this PR: 18.07, 18.72, 19.01

incremental build baseline: 3.63, 3.46, 3.63
incremental build this PR: 2.65, 2.77, 2.85

CodaFi · 2021-09-09T22:33:34Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

-      ])
-      self.abbreviate(.moduleDepGraphNode, [
-        .literal(RecordID.moduleDepGraphNode.rawValue),
+      let dependencyKeyOperands: [Bitstream.Abbreviation.Operand] = [


Is this factorization relevant to the patch?

The old code duplicated this information, and yes, I could have done the patch by keeping the duplication. Since the patch involved changing the content of the serialized key, the factorization contributed to the quality of the patch by centralizing the invariant around what was needed in the file.

Someday, I'd love to further centralize so that the structure, the code to read, and the code to write each type of node were factored together.

In the meantime, the round-trip test you wrote provides a way to catch many of the potential bugs.

CodaFi · 2021-09-09T22:39:29Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

+          let s = try internedString(field: i)
+          return s.isEmpty ? nil : s
+        }
+        func dependencyKey(fields: (kindCode: Int,


Nit: The use of a tuple makes the call sites opaque. Perhaps this should be unpacked back out into separate operands in the signature.

Easy enough to fix!

CodaFi · 2021-09-09T22:41:51Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+
+extension InternedString {
+  public static func < (lhs: InternedString, rhs: InternedString) -> Bool {
+    lhs.index < rhs.index


This doesn't seem correct. And if it is, it's a very surprising implementation of comparison for this type. I would expect that we would force callers to go through isInIncreasingOrder et al.

Yes, a holdover. It's gone.

CodaFi · 2021-09-09T22:42:08Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+    return r
+  }
+
+  public func lookup(`in` holder: InternedStringTableHolder) -> String {


Nit: in does not need to be escaped here.

Thanks. Done!

CodaFi · 2021-09-09T22:43:56Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+
+  public init() {}
+
+  fileprivate func index(_ s: String) -> Int {


Nit: This is a mutating operation with a name that does not convey as much. I would expect this to be named intern as well.

Sure, no prob.

CodaFi · 2021-09-09T22:58:13Z

Some general comments:

Thread safety is a pretty big concern here. Could you try running your profiling with a serial access queue attached to see what the overhead of synchronization is?
isInIncreasingOrder doesn't appear to be used anywhere. I suspect we can drop it from this patch and introduce it later when you add it to the Tracer.
The current representation gives callers direct access to the Int in the handle. That should be opaque, and it should not be possible for a caller to construct an interned string handle themselves without going through the table.

CodaFi · 2021-09-09T23:02:29Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+//
+//===----------------------------------------------------------------------===//
+
+import Foundation


Nit: I don't believe we need this import.

Thanks. Gone!

CodaFi · 2021-09-09T23:04:18Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+  var strings = [""]
+  fileprivate var indices = ["": 0]
+
+  public init() {}


Please also benchmark the high water mark for this table and see if reserveCapacity on both collections helps here.

I assume you mean to save the table size in the priors, and reserve it when reading the priors. Great idea! I'll do that.

davidungar · 2021-09-10T00:08:10Z

@swift-ci please test

davidungar · 2021-09-23T03:17:51Z

@swift-ci please test

davidungar · 2021-09-23T04:07:07Z

@artemcm @CodaFi I have added thread-safety in the form of dispatchPrecondition as suggested. Seems to be very little cost. How does this look now? Thanks!

CodaFi · 2021-09-24T18:21:03Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationSynchronizer.swift

+/// Any instance that can find the confinmentQueue for the incremental compilation state.
+/// By confirming, a type gains the use of the shorthand methods in the extension.
+public protocol IncrementalCompilationSynchronizer {
+  var incrementalCompilationQueue: DispatchQueue {get}


Why not just have the table own a confinement queue, or be accessible only from behind a class (say the module dependency graph) that protects it with a confinement queue? By introducing this extra axis of customizability, the it's harder to reason about the concurrency guarantees for the table since the queue it is accessed from could be (but, in this patch, is not after manual inspection) distinct from the one it is initialized with.

Yes, an earlier version of this PR required a module dependency graph to be passed in to every intern / un-intern operation. IIRC, @artemcm correctly thought that code was a bit unclear. I can do away with the protocol if that would help. Again, for the sake of realizing the performance gains, I would love to find an architecture that would be above threshold for you. What can you suggest? I can try going back to the string-table hidden in the module dependency graph, but then operations such as reading a source file dependency graph for testing, or reading the priors will have to start from an empty module dependency graph. Not the worst thing in the world, but also a source of awkwardness, passing in a whole graph just to gain the ability to intern strings.

CodaFi · 2021-09-24T18:32:46Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+  public var isEmpty: Bool { index == 0 }
+
+  // PRIVATE?
+  init(index: Int) {


Yes, this must be private, or the handle needs to become an opaque type a la VirtualPath.Handle. Callers should never be able to construct these things without going through the table.

When reading a serialized ModuleDependencyGraph, this PR saves work by using the indices of identifiers in the serialized graph as the indices into the InternedStringTable. So, there must be a bridge from the method in the ModuleDependencyGraph.swift code into this method, because the reader must take an index and turn it into an InternedString.

Can you suggest something better? Something like a C++ friend would be great.

this PR saves work by using the indices of identifiers in the serialized graph as the indices into the InternedStringTable

I'm not sure it is worth tying these concepts together? It is not much work to have a table that just increments an index for every added string. And tying creation of interned strings to the table has a benefit of this mechanism always being explicit so that clients only have one way to deal with this, as Robert suggests.

It would also simplify this patch a bit, this doesn't seem like the kind of change that calls for modifying the serialization format in addition to adding string interning and changing the synchronization mechanisms.

It seems that ModuleDependencyGraph/deserialize/Visitor already has a table it is building up, so we would be able to make an interned string's index to be private and rid of this init.

CodaFi · 2021-09-24T18:34:29Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift

  }

-  /// Block any threads from mutating `ProtectedState`
-  public func blockingConcurrentMutation<R>(
+  /// Allow concurrent access to while preventing mutation of ``IncrementalCompilationState/protectedState``


Nit: sentence fragment

Also the in-source link here isn't valid. DocC doesn't support (at least, ergonomically) linking to instance variables like this. A plain protectedState will do.

Thanks, done!

CodaFi · 2021-09-24T18:38:41Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationState.swift


+  /// State needed for incremental compilation that can change during a run and must be protected from
+  /// concurrent mutation and access. Concurrent accesses are OK.
  private var protectedState: ProtectedState


The protected state should not have to include info

CodaFi · 2021-09-24T18:38:58Z

Sources/SwiftDriver/IncrementalCompilation/IncrementalCompilationProtectedState.swift

-  }
-  fileprivate func checkAccess() {
-    dispatchPrecondition(condition: .onQueue(confinmentQueue))
-  }
 }

 // MARK: - 2nd wave
 extension IncrementalCompilationState.ProtectedState {
  mutating func collectBatchedJobsDiscoveredToBeNeededAfterFinishing(


Please mark this fileprivate so it cannot be accessed without going through the confinement queue.

CodaFi · 2021-09-24T18:39:58Z

Seems to be very little cost.

Do you have numbers here? The confinement queues shouldn't add that much overhead, but it should still appear in traces.

davidungar · 2021-10-07T17:20:27Z

Seems to be very little cost.

Do you have numbers here? The confinement queues shouldn't add that much overhead, but it should still appear in traces.

Fair question: Just to be sure remeasured, using Instruments and filtering the tree on symbols matching "dispatchPrecondition", inverting the call tree.
Results:

testCleanBuildPerformance: 1ms out of 2.37 s
clean build of a project with more 1,000 source files: 9 ms out of 16.3 s for the driver
quasi-null build (just one small change) of the same project: 6 ms out of 2.5 s for the driver.

davidungar · 2021-10-07T23:01:49Z

@swift-ci please test

davidungar · 2021-10-07T23:03:59Z

@swift-ci please test

davidungar · 2021-10-08T18:29:16Z

@artem & @CodaFi : I think this is as far as I can go on this without more input from you. Would you care to take another look? Thanks.

artemcm · 2021-10-08T20:43:22Z

Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift


 /// A filename from another module
 /*@_spi(Testing)*/ final public class ExternalDependency: Hashable, Comparable, CustomStringConvertible {


  /// Delay computing the path as an optimization.
-  let fileName: String
+  let fileName: InternedString


It seems like we don't really need the InternedString here if we must have its raw value stored?

If I interpret Xcode's callers facility correctly, the fileName is used in lots of places. Storing a String here would add some amount of overhead, as well as needing the string table at the use point.

artemcm · 2021-10-08T20:49:40Z

Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift

+  default: break
+  }
+
+  /// Preserves the ordering that obtained before interned strings were introduced.


The reader of this code probably shouldn't care too much about the context behind introducing this versus why is it that we need this ordering in the first place?

Not sure how to address the concern here.

artemcm · 2021-10-08T20:54:07Z

Sources/SwiftDriver/IncrementalCompilation/DependencyKey.swift

+  /// Preserves the ordering that obtained before interned strings were introduced.
+  func kindOrdering(_ d: DependencyKey.Designator) -> Int {
+    switch d {
+    case .topLevel: return 1


Alternatively, you can make Designator conform to Comparable and implement this there to avoid exposing this kind of detail?

The issue is needing the string table as a parameter to the less-than function.

artemcm · 2021-10-08T21:15:52Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

@@ -606,6 +657,7 @@ extension ModuleDependencyGraph {
    case malformedIdentifierRecord
    case malformedModuleDepGraphNodeRecord
    case malformedDependsOnRecord
+    case malforedUseIDRecord


Suggested change

case malforedUseIDRecord

case malformedUseIDRecord

artemcm · 2021-10-08T21:16:42Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraph.swift

+      case .dependsOnNode:
+        self = .malformedDependsOnRecord
+      case .useIDNode:
+        self = .malforedUseIDRecord


Suggested change

self = .malforedUseIDRecord

self = .malformedUseIDRecord

Fixed or rendered moot.

artemcm · 2021-10-08T21:40:35Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+  public var isEmpty: Bool { index == 0 }
+
+  // PRIVATE?
+  init(index: Int) {


this PR saves work by using the indices of identifiers in the serialized graph as the indices into the InternedStringTable

I'm not sure it is worth tying these concepts together? It is not much work to have a table that just increments an index for every added string. And tying creation of interned strings to the table has a benefit of this mechanism always being explicit so that clients only have one way to deal with this, as Robert suggests.

It would also simplify this patch a bit, this doesn't seem like the kind of change that calls for modifying the serialization format in addition to adding string interning and changing the synchronization mechanisms.

artemcm · 2021-10-08T21:57:13Z

Sources/SwiftDriver/IncrementalCompilation/ModuleDependencyGraphParts/InternedStrings.swift

+  public var isEmpty: Bool { index == 0 }
+
+  // PRIVATE?
+  init(index: Int) {


It seems that ModuleDependencyGraph/deserialize/Visitor already has a table it is building up, so we would be able to make an interned string's index to be private and rid of this init.

davidungar · 2021-10-13T00:25:35Z

After an offline discussion, we agreed it would be robust enough to make the init(index:) constructor private to the deserialization code.

davidungar · 2021-10-13T00:34:16Z

@swift-ci please test

davidungar · 2021-10-13T17:36:41Z

Also helps rdar://79192750

davidungar marked this pull request as draft August 17, 2021 21:56

davidungar force-pushed the intern-experiment branch 3 times, most recently from d09932f to 5a33277 Compare August 18, 2021 22:04

davidungar changed the title ~~[WIP, DNM, Incremental] Intern experiment for performance~~ [Incremental] Optimize incremental build logic by interning strings Aug 19, 2021

davidungar marked this pull request as ready for review August 19, 2021 20:47

artemcm reviewed Aug 19, 2021

View reviewed changes

davidungar force-pushed the intern-experiment branch from 444b6e1 to ba887d1 Compare September 9, 2021 20:46

davidungar requested a review from CodaFi September 9, 2021 20:50

CodaFi reviewed Sep 9, 2021

View reviewed changes

davidungar force-pushed the intern-experiment branch from d285529 to e4047fb Compare September 22, 2021 20:50

CodaFi reviewed Sep 24, 2021

View reviewed changes

David Ungar added 12 commits October 7, 2021 10:23

graph abstraction

cdcf32b

fix tests

8cc757a

add write priors test

9a4a168

add deserialization performance test

ed61a34

intern strings, etc.

6f14d99

Fix the easy stuff.

2c7a81a

Move confinementQueue to info

51141a7

Get things working

1f0aab9

wip

a0e24de

Fix comment

81fe7fc

fix tests to compile

8794e8a

Fix fingerprint disposition remark.

b674cdf

davidungar force-pushed the intern-experiment branch from 6c2001a to b674cdf Compare October 7, 2021 22:32

Explain

f71b7e8

artemcm reviewed Oct 8, 2021

View reviewed changes

Addressed review comments.

d2c805d

artemcm approved these changes Oct 13, 2021

View reviewed changes

davidungar merged commit 51233ed into swiftlang:main Oct 13, 2021


		public init() {}

		fileprivate func index(_ s: String) -> Int {

[Incremental] Optimize incremental build logic by interning strings #800

[Incremental] Optimize incremental build logic by interning strings #800

Conversation

davidungar commented Aug 17, 2021

davidungar commented Aug 17, 2021

davidungar commented Aug 19, 2021

davidungar commented Aug 19, 2021

davidungar commented Aug 19, 2021

davidungar commented Aug 19, 2021

davidungar commented Aug 19, 2021

davidungar commented Aug 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidungar commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented Sep 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidungar commented Sep 10, 2021

davidungar commented Sep 23, 2021

davidungar commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodaFi commented Sep 24, 2021

davidungar commented Oct 7, 2021

davidungar commented Oct 7, 2021

davidungar commented Oct 7, 2021

davidungar commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidungar commented Oct 13, 2021

davidungar commented Oct 13, 2021

davidungar commented Oct 13, 2021

CodaFi Sep 9, 2021 •

edited

Loading

CodaFi Sep 9, 2021 •

edited

Loading