Implement a better cache key for resolved references (#793)

Author: d-ronnqvist

Sources/SwiftDocC/Model/Identifier.swift

@@ -141,10 +141,14 @@ extension TopicReferenceResolutionErrorInfo {
 /// > its data.
 public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomStringConvertible {
     typealias ReferenceBundleIdentifier = String
-    typealias ReferenceKey = String
+    private struct ReferenceKey: Hashable {
+        var path: String
+        var fragment: String?
+        var sourceLanguages: Set<SourceLanguage>
+    }
 
     /// A synchronized reference cache to store resolved references.
-    static var sharedPool = Synchronized([ReferenceBundleIdentifier: [ReferenceKey: ResolvedTopicReference]]())
+    private static var sharedPool = Synchronized([ReferenceBundleIdentifier: [ReferenceKey: ResolvedTopicReference]]())
 
     /// Clears cached references belonging to the bundle with the given identifier.
     /// - Parameter bundleIdentifier: The identifier of the bundle to which the method should clear belonging references.
@@ -219,11 +223,7 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
     private init(bundleIdentifier: String, urlReadablePath: String, urlReadableFragment: String? = nil, sourceLanguages: Set<SourceLanguage>) {
         precondition(!sourceLanguages.isEmpty, "ResolvedTopicReference.sourceLanguages cannot be empty")
         // Check for a cached instance of the reference
-        let key = Self.cacheKey(
-            urlReadablePath: urlReadablePath,
-            urlReadableFragment: urlReadableFragment,
-            sourceLanguages: sourceLanguages
-        )
+        let key = ReferenceKey(path: urlReadablePath, fragment: urlReadableFragment, sourceLanguages: sourceLanguages)
         let cached = Self.sharedPool.sync { $0[bundleIdentifier]?[key] }
         if let resolved = cached {
             self = resolved
@@ -244,20 +244,6 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
         }
     }
 
-    private static func cacheKey(
-        urlReadablePath path: String,
-        urlReadableFragment fragment: String?,
-        sourceLanguages: Set<SourceLanguage>
-    ) -> String {
-        let sourceLanguagesString = sourceLanguages.map(\.id).sorted().joined(separator: "-")
-        
-        if let fragment = fragment {
-            return "\(path):\(fragment):\(sourceLanguagesString)"
-        } else {
-            return "\(path):\(sourceLanguagesString)"
-        }
-    }
-    
     /// The topic URL as you would write in a link.
     public var url: URL {
         return _storage.url
@@ -495,22 +481,10 @@ public struct ResolvedTopicReference: Hashable, Codable, Equatable, CustomString
             self.absoluteString = self.url.absoluteString
         }
     }
-}
-
-typealias ResolvedTopicReferenceCacheKey = String
-
-extension ResolvedTopicReference {
-    /// Returns a unique cache ID for a pair of unresolved and parent references.
-    static func cacheIdentifier(_ reference: UnresolvedTopicReference, fromSymbolLink: Bool, in parent: ResolvedTopicReference?) -> ResolvedTopicReferenceCacheKey {
-        let isSymbolLink = fromSymbolLink ? ":symbol" : ""
-        if let parent = parent {
-            // Create a cache id in the parent context
-            return "\(reference.topicURL.absoluteString):\(parent.bundleIdentifier):\(parent.path):\(parent.sourceLanguage.id)\(isSymbolLink)"
-        } else {
-            // A cache ID for an external reference
-            assert(reference.topicURL.components.host != nil)
-            return reference.topicURL.absoluteString.appending(isSymbolLink)
-        }
+    
+    // For testing the caching
+    static func _numberOfCachedReferences(bundleID: ReferenceBundleIdentifier) -> Int? {
+        return Self.sharedPool.sync { $0[bundleID]?.count }
     }
 }
 

Tests/SwiftDocCTests/Converter/RenderNodeCodableTests.swift

@@ -146,21 +146,14 @@ class RenderNodeCodableTests: XCTestCase {
             subdirectory: "Test Resources"
         )!
 
-        let uniqueBundleIdentifier = #function
+        let bundleID = #function
 
-        let renderNodeWithUniqueBundleID = try String(
-            contentsOf: exampleRenderNodeJSON
-        )
-        .replacingOccurrences(
-            of: "org.swift.docc.example",
-            with: uniqueBundleIdentifier
-        )
+        let renderNodeWithUniqueBundleID = try String(contentsOf: exampleRenderNodeJSON)
+        .replacingOccurrences(of: "org.swift.docc.example", with: bundleID)
 
         _ = try JSONDecoder().decode(RenderNode.self, from: Data(renderNodeWithUniqueBundleID.utf8))
 
-        ResolvedTopicReference.sharedPool.sync { sharedPool in
-            XCTAssertNil(sharedPool[uniqueBundleIdentifier])
-        }
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID))
     }
 
     func testDecodeRenderNodeWithoutTopicSectionStyle() throws {

Tests/SwiftDocCTests/Indexing/NavigatorIndexTests.swift

@@ -260,9 +260,7 @@ Root
             atomically: true
         )
 
-        ResolvedTopicReference.sharedPool.sync { sharedPool in
-            XCTAssertNil(sharedPool[uniqueTestBundleIdentifier])
-        }
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: uniqueTestBundleIdentifier))
     }
 
 

Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift

@@ -2672,41 +2672,35 @@ let expected = """
     }
 
     func testContextCachesReferences() throws {
+        let bundleID = #function
         // Verify there is no pool bucket for the bundle we're about to test
-        XCTAssertNil(ResolvedTopicReference.sharedPool.sync({ $0[#function] }))
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID))
 
         let (_, _, _) = try testBundleAndContext(copying: "TestBundle", excludingPaths: [], codeListings: [:], configureBundle: { rootURL in
             let infoPlistURL = rootURL.appendingPathComponent("Info.plist", isDirectory: false)
             try! String(contentsOf: infoPlistURL)
-                .replacingOccurrences(of: "org.swift.docc.example", with: #function)
+                .replacingOccurrences(of: "org.swift.docc.example", with: bundleID)
                 .write(to: infoPlistURL, atomically: true, encoding: .utf8)
         })
 
         // Verify there is a pool bucket for the bundle we've loaded
-        XCTAssertNotNil(ResolvedTopicReference.sharedPool.sync({ $0[#function] }))
+        XCTAssertNotNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID))
 
-        guard let references = ResolvedTopicReference.sharedPool.sync({ $0[#function] }) else {
-            return
-        }
-        
-        let beforeCount = references.count
+        let beforeCount = try XCTUnwrap(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID))
 
         // Verify a given identifier exists in the pool by creating it and verifying it wasn't added to the pool
-        let identifier = ResolvedTopicReference(bundleIdentifier: #function, path: "/tutorials/Test-Bundle/TestTutorial", sourceLanguage: .swift)
-        _ = identifier
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/tutorials/Test-Bundle/TestTutorial", sourceLanguage: .swift)
 
         // Verify create the reference above did not add to the cache
-        XCTAssertEqual(beforeCount, ResolvedTopicReference.sharedPool.sync({ $0[#function]!.count }))
+        XCTAssertEqual(beforeCount, ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID))
 
         // Create a new reference for the same bundle that was not loaded with the context
-        let newIdentifier = ResolvedTopicReference(bundleIdentifier: #function, path: "/tutorials/Test-Bundle/TestTutorial/\(#function)", sourceLanguage: .swift)
-        _ = newIdentifier
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/tutorials/Test-Bundle/TestTutorial/\(#function)", sourceLanguage: .swift)
 
         // Verify creating a new reference added to the ones loaded with the context
-        XCTAssertNotEqual(beforeCount, ResolvedTopicReference.sharedPool.sync({ $0[#function]!.count }))
+        XCTAssertNotEqual(beforeCount, ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID))
 
-        // Purge the pool
-        ResolvedTopicReference.purgePool(for: #function)
+        ResolvedTopicReference.purgePool(for: bundleID)
     }
 
     func testAbstractAfterMetadataDirective() throws {

Tests/SwiftDocCTests/Model/IdentifierTests.swift

@@ -65,67 +65,42 @@ class IdentifierTests: XCTestCase {
     }
 
     func testReusingReferences() {
-        // Verify the bundle doesn't exist in the pool
-        XCTAssertFalse(ResolvedTopicReference.sharedPool.sync({ $0.keys.contains(#function) }))
+        let bundleID = #function
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), "Cache for this bundle shouldn't exist because caching is not enabled by default")
 
-        // Enable caching for our test bundle identifier
-        ResolvedTopicReference.enableReferenceCaching(for: #function)
+        // Add one reference
+        ResolvedTopicReference.enableReferenceCaching(for: bundleID)
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), 0, "Should have an empty cache after enabling reference caching for this bundle")
 
-        // Create a resolved reference
-        let ref = ResolvedTopicReference(bundleIdentifier: #function, path: "/path/child", sourceLanguage: .swift)
-        _ = ref // to suppress the warning above
+        // Add the same reference repeatedly
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/path/to/page", sourceLanguage: .swift)
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), 1, "Should have an cached one reference because a reference with this bundle identifier was created")
 
-        // Verify the bundle was added to the pool
-        guard let references = ResolvedTopicReference.sharedPool.sync({ $0[#function] }) else {
-            XCTFail("Reference bundle was not added to reference cache")
-            return
-        }
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/path/to/page", sourceLanguage: .swift)
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/path/to/page", sourceLanguage: .swift)
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), 1, "Should still only have one cached reference because the same reference was created repeatedly")
 
-        // Verify the child is now in the pool
-        XCTAssertEqual(references.contains(where: { pair -> Bool in
-            return pair.key.contains("/path/child")
-        }), true)
+        // Add another reference
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/path/to/other-page", sourceLanguage: .swift)
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), 2, "Should have cached another reference because two different references with this bundle identifier has been created")
 
-        // Clear the cache
-        ResolvedTopicReference.purgePool(for: #function)
+        // Purge and repeat
+        ResolvedTopicReference.purgePool(for: bundleID)
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), "Cache for this bundle shouldn't have been deleted because the pool was purged")
 
-        // Verify there are no references in the pool for that bundle
-        XCTAssertFalse(ResolvedTopicReference.sharedPool.sync({ $0.keys.contains(#function) }))
+        ResolvedTopicReference.enableReferenceCaching(for: bundleID)
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), 0, "Should have an empty cache after enabling reference caching for this bundle")
 
-        // Re-enable caching for our test bundle identifier
-        ResolvedTopicReference.enableReferenceCaching(for: #function)
-        
-        let ref1 = ResolvedTopicReference(bundleIdentifier: #function, path: "/path/child", sourceLanguage: .swift)
-        _ = ref1
-        
-        // Verify the bundle was added to the pool
-        guard let references1 = ResolvedTopicReference.sharedPool.sync({ $0[#function] }) else {
-            XCTFail("Reference bundle was not added to reference cache")
-            return
-        }
-
-        // Verify the pool bucket was re-created and the reference is in the pool
-        XCTAssertEqual(references1.contains(where: { pair -> Bool in
-            return pair.key.contains("/path/child")
-        }), true)
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/path/to/page", sourceLanguage: .swift)
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), 1, "Should have an cached one reference because a reference with this bundle identifier was created")
     }
 
     func testReferencesAreNotCachedByDefault() {
-        // Verify the bundle doesn't exist in the pool
-        XCTAssertFalse(ResolvedTopicReference.sharedPool.sync({ $0.keys.contains(#function) }))
-        
-        // Create a resolved reference
-        let reference = ResolvedTopicReference(
-            bundleIdentifier: #function,
-            path: "/path/child",
-            sourceLanguage: .swift
-        )
-        
-        // Verify the bundle still doesn't exist in the pool
-        XCTAssertFalse(ResolvedTopicReference.sharedPool.sync({ $0.keys.contains(#function) }))
+        let bundleID = #function
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), "References for this bundle shouldn't exist because caching is not enabled by default")
 
-        // Add a use of 'reference' to suppress Swift's 'reference' was never used warning
-        XCTAssertNotNil(reference)
+        _ = ResolvedTopicReference(bundleIdentifier: bundleID, path: "/path/to/page", sourceLanguage: .swift)
+        XCTAssertNil(ResolvedTopicReference._numberOfCachedReferences(bundleID: bundleID), "After creating a reference in this bundle, references still shouldn't exist because caching is not enabled by default")
     }
 
     func testReferenceInitialPathComponents() {

Tests/SwiftDocCUtilitiesTests/ConvertActionTests.swift

@@ -1769,9 +1769,7 @@ class ConvertActionTests: XCTestCase {
 
         _ = try action.perform(logHandle: .none)
 
-        ResolvedTopicReference.sharedPool.sync { sharedPool in
-            XCTAssertEqual(sharedPool[#function]?.count, 8)
-        }
+        XCTAssertEqual(ResolvedTopicReference._numberOfCachedReferences(bundleID: #function), 8)
     }
 
     func testIgnoresAnalyzerHintsByDefault() throws {