Change drop parameter in LookupCache and set a cap to prevent overgrowing cache. Refactoring.

Author: MAJKFL

Sources/SwiftLexicalLookup/LookupCache.swift

@@ -22,34 +22,24 @@ public class LookupCache {
   /// Identified by `SyntaxIdentifier`.
   private let sequentialResultsCache: LRUCache<SyntaxIdentifier, [LookupResult]>
   /// Looked-up scope identifiers during cache accesses.
-  private var hits: Set<SyntaxIdentifier> = []
-
-  private let dropMod: Int
+  private var hits: Set<SyntaxIdentifier> = [] {
+    didSet {
+      if hits.count > capacity * 2 {
+        hits.removeAll()
+      }
+    }
+  }
+  
+  private let capacity: Int
   private var evictionCount = 0
 
   /// Creates a new unqualified lookup cache.
   /// `capacity` describes the maximum amount of entries in the cache.
   /// The cache size is maintained according to the LRU (Least Recently Used) policy.
-  /// `drop` parameter specifies how many eviction calls will be
-  /// ignored before evicting not-hit members from subsequent lookups.
-  ///
-  /// Example cache eviction sequences (s - skip, e - evict):
-  /// - `drop = 0` - `e -> e -> e -> e -> e -> ...`
-  /// - `drop = 1` - `s -> e -> s -> s -> e -> ...`
-  /// - `drop = 3` - `s -> s -> s -> e -> s -> ...`
-  ///
-  /// - Note: `drop = 0` effectively maintains exactly one path of cached results to
-  /// the root in the cache (assuming we evict cache members after each lookup in a sequence of lookups).
-  /// Higher the `drop` value, more such paths can potentially be stored in the cache at any given moment.
-  /// Because of that, a higher `drop` value also translates to a higher number of cache-hits,
-  /// but it might not directly translate to better performance. Because of a larger memory footprint,
-  /// memory accesses could take longer, slowing down the eviction process. That's why the `drop` value
-  /// could be fine-tuned to maximize the performance given file size,
-  /// number of lookups, and amount of available memory.
-  public init(capacity: Int, drop: Int = 0) {
+  public init(capacity: Int) {
+    self.capacity = capacity
     self.ancestorResultsCache = LRUCache(capacity: (capacity + 1) / 2)
     self.sequentialResultsCache = LRUCache(capacity: capacity / 2)
-    self.dropMod = drop + 1
   }
 
   /// Get cached ancestor results for the given `id`.
@@ -86,17 +76,31 @@ public class LookupCache {
   }
 
   /// Removes all cached entries without a hit, unless it's prohibited
-  /// by the internal drop counter (as specified by `drop` in the initializer).
-  /// The dropping behavior can be disabled for this call with the `bypassDropCounter`
-  /// parameter, resulting in immediate eviction of entries without a hit.
-  public func evictEntriesWithoutHit(bypassDropCounter: Bool = false) {
-    if !bypassDropCounter {
-      evictionCount = (evictionCount + 1) % dropMod
-      guard evictionCount != 0 else { return }
-    }
+  /// by the internal `drop` counter.
+  /// `drop` parameter specifies how many eviction calls will be
+  /// ignored before evicting not-hit members from subsequent lookups.
+  ///
+  /// Example cache eviction sequences (s - skip, e - evict):
+  /// - `drop = 0` - `e -> e -> e -> e -> e -> ...`
+  /// - `drop = 1` - `s -> e -> s -> s -> e -> ...`
+  /// - `drop = 3` - `s -> s -> s -> e -> s -> ...`
+  ///
+  /// - Note: `drop = 0` effectively maintains exactly one path of cached results to
+  /// the root in the cache (assuming we evict cache members after each lookup in a sequence of lookups).
+  /// Higher the `drop` value, more such paths can potentially be stored in the cache at any given moment.
+  /// Because of that, a higher `drop` value also translates to a higher number of cache-hits,
+  /// but it might not directly translate to better performance. Because of a larger memory footprint,
+  /// memory accesses could take longer, slowing down the eviction process. That's why the `drop` value
+  /// could be fine-tuned to maximize the performance given file size,
+  /// number of lookups, and amount of available memory.
+  public func evictEntriesWithoutHit(drop: Int = 0) {
+    evictionCount = (evictionCount + 1) % (drop + 1)
+    guard evictionCount != 0 else { return }
 
-    for key in ancestorResultsCache.keysInCache.union(sequentialResultsCache.keysInCache).subtracting(hits) {
+    for key in ancestorResultsCache.keys where !hits.contains(key) {
       ancestorResultsCache[key] = nil
+    }
+    for key in sequentialResultsCache.keys where !hits.contains(key) {
       sequentialResultsCache[key] = nil
     }
 

Sources/SwiftLexicalLookup/Scopes/ScopeImplementations.swift

@@ -379,7 +379,9 @@ import SwiftSyntax
     with config: LookupConfig,
     cache: LookupCache?
   ) -> [LookupResult] {
-    // We're not using `lookupParent` to not cache the results here.
+    // We're not using `lookupParent` to not cache sequential parent results here.
+    // Sequential scope might filter out specific names based on the location within this guard stmt
+    // in subsequent lookups. We're relying on cache of the parent sequential scope instead.
     parentScope?.lookup(identifier, at: lookUpPosition, with: config, cache: cache) ?? []
   }
 }
@@ -732,7 +734,7 @@ import SwiftSyntax
   /// the function returns exactly two results. First associated with the member
   /// block that consists of the `associatedtype A` declaration and
   /// the latter one from the file scope and `class A` exactly in this order.
-  public func lookup(
+  @_spi(Experimental) public func lookup(
     _ identifier: Identifier?,
     at lookUpPosition: AbsolutePosition,
     with config: LookupConfig,
@@ -995,7 +997,9 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
         cache: cache
       )
     } else {
-      // We're not using `lookupParent` to not cache the results here.
+      // We're not using `lookupParent` to not cache sequential parent results here.
+      // Sequential scope might filter out specific names based on the location within this variable decl
+      // in subsequent lookups. We're relying on cache of the parent sequential scope instead.
       return parentScope?.lookup(identifier, at: lookUpPosition, with: config, cache: cache) ?? []
     }
   }
@@ -1123,7 +1127,9 @@ extension SubscriptDeclSyntax: WithGenericParametersScopeSyntax, CanInterleaveRe
     with config: LookupConfig,
     cache: LookupCache?
   ) -> [LookupResult] {
-    // We're not using `lookupParent` to not cache the results here.
+    // We're not using `lookupParent` to not cache sequential parent results here.
+    // Sequential scope might filter out specific names based on the location within this if config
+    // in subsequent lookups. We're relying on cache of the parent sequential scope instead.
     parentScope?.lookup(identifier, at: lookUpPosition, with: config, cache: cache) ?? []
   }
 }

Sources/SwiftLexicalLookup/Scopes/ScopeSyntax.swift

@@ -50,57 +50,57 @@ extension SyntaxProtocol {
     with config: LookupConfig = LookupConfig(),
     cache: LookupCache? = nil
   ) -> [LookupResult] {
-    if let cache, let identifier {
-      let filteredResult: [LookupResult] = (scope?.lookup(nil, at: self.position, with: config, cache: cache) ?? [])
-        .compactMap { result in
-          switch result {
-          case .fromScope(let syntax, let withNames):
-            let filteredNames =
-              withNames
-              .filter { name in
-                name.identifier == identifier
-              }
-
-            guard filteredNames.count != 0 else { return nil }
-            return .fromScope(syntax, withNames: filteredNames)
-          default:
-            return result
-          }
+    guard let cache, let identifier else {
+      return scope?.lookup(identifier, at: self.position, with: config, cache: cache) ?? []
+    }
+    
+    let filteredResult: [LookupResult] = (scope?.lookup(nil, at: self.position, with: config, cache: cache) ?? [])
+      .compactMap { result in
+        switch result {
+        case .fromScope(let syntax, let withNames):
+          let filteredNames =
+            withNames
+            .filter { name in
+              name.identifier == identifier
+            }
+
+          guard filteredNames.count != 0 else { return nil }
+          return .fromScope(syntax, withNames: filteredNames)
+        default:
+          return result
         }
+      }
+
+    var resultWithMergedSequentialResults: [LookupResult] = []
+    var i = 0
 
-      var resultWithMergedSequentialResults: [LookupResult] = []
-      var i = 0
+    while i < filteredResult.count {
+      let thisResult = filteredResult[i]
 
-      while i < filteredResult.count {
-        let thisResult = filteredResult[i]
+      if i < filteredResult.count - 1,
+        case .fromScope(let thisScope, let thisNames) = thisResult,
+        thisScope.asProtocol(SyntaxProtocol.self) is SequentialScopeSyntax
+      {
+        var accumulator = thisNames
 
-        if i < filteredResult.count - 1,
-          case .fromScope(let thisScope, let thisNames) = thisResult,
-          thisScope.asProtocol(SyntaxProtocol.self) is SequentialScopeSyntax
+        while i < filteredResult.count - 1,
+          case .fromScope(let otherScope, let otherNames) = filteredResult[i + 1],
+          otherScope.asProtocol(SyntaxProtocol.self) is SequentialScopeSyntax,
+          thisScope.id == otherScope.id
         {
-          var accumulator = thisNames
-
-          while i < filteredResult.count - 1,
-            case .fromScope(let otherScope, let otherNames) = filteredResult[i + 1],
-            otherScope.asProtocol(SyntaxProtocol.self) is SequentialScopeSyntax,
-            thisScope.id == otherScope.id
-          {
-            accumulator += otherNames
-            i += 1
-          }
-
-          resultWithMergedSequentialResults.append(.fromScope(thisScope, withNames: accumulator))
-        } else {
-          resultWithMergedSequentialResults.append(thisResult)
+          accumulator += otherNames
+          i += 1
         }
 
-        i += 1
+        resultWithMergedSequentialResults.append(.fromScope(thisScope, withNames: accumulator))
+      } else {
+        resultWithMergedSequentialResults.append(thisResult)
       }
 
-      return resultWithMergedSequentialResults
-    } else {
-      return scope?.lookup(identifier, at: self.position, with: config, cache: cache) ?? []
+      i += 1
     }
+
+    return resultWithMergedSequentialResults
   }
 }
 

Sources/SwiftSyntax/LRUCache.swift

@@ -31,22 +31,24 @@ package class LRUCache<Key: Hashable, Value> {
   private unowned var head: _Node?
   private unowned var tail: _Node?
 
-  public let capacity: Int
-  public private(set) var keysInCache: Set<Key>
+  package let capacity: Int
 
-  public init(capacity: Int) {
+  package init(capacity: Int) {
     self.table = [:]
     self.head = nil
     self.tail = nil
     self.capacity = capacity
-    self.keysInCache = []
   }
 
-  public var count: Int {
+  package var count: Int {
     return table.count
   }
+  
+  package var keys: some Collection<Key> {
+    table.keys
+  }
 
-  public subscript(key: Key) -> Value? {
+  package subscript(key: Key) -> Value? {
     get {
       guard let node = table[key] else {
         return nil
@@ -61,7 +63,6 @@ package class LRUCache<Key: Hashable, Value> {
         self.ensureCapacityForNewValue()
         let node = _Node(key: key, value: newValue)
         addToHead(node: node)
-        keysInCache.insert(key)
         table[key] = node
 
       case let (node?, newValue?):  // update.
@@ -70,7 +71,6 @@ package class LRUCache<Key: Hashable, Value> {
 
       case let (node?, nil):  // delete.
         remove(node: node)
-        keysInCache.remove(key)
         table[key] = nil
 
       case (nil, nil):  // no-op.

Tests/SwiftLexicalLookupTest/Assertions.swift

@@ -147,6 +147,7 @@ private func testFunction(
   let lookupIdentifier = Identifier(tokenAtMarker)
 
   let result = tokenAtMarker.lookup(useNilAsTheParameter ? nil : lookupIdentifier, with: config, cache: cache)
+  cache?.evictEntriesWithoutHit(drop: 3)
 
   guard let expectedValues = references[marker] else {
     XCTFail("For marker \(marker), couldn't find result expectation")

Tests/SwiftCompilerPluginTest/LRUCacheTests.swift renamed to Tests/SwiftSyntaxTest/LRUCacheTests.swift

@@ -10,7 +10,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-@_spi(Testing) import SwiftCompilerPluginMessageHandling
 import SwiftSyntax
 import XCTest