From 33250e5db55c23c56dd73e054be35dcba36169e8 Mon Sep 17 00:00:00 2001 From: Roman Laitarenko Date: Mon, 26 Aug 2024 16:09:53 +0300 Subject: [PATCH] Fix memory leak in partial GeoJSON update (#2268) --- CHANGELOG.md | 2 + .../MapboxMaps/Style/StyleSourceManager.swift | 53 +++++++++++++------ .../Style/StyleSourceManagerTests.swift | 25 +++++++++ 3 files changed, 65 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc14199c1463..f97335e3168e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ Mapbox welcomes participation and contributions from everyone. ## main +* Improve memory reclamation behavior when using partial GeoJSON update API. + ## 11.6.0 - 14 August, 2024 * Expose getters for `MapOptions.orientation`, `MapOptions.constrainMode` and `MapOptions.viewportMode`. diff --git a/Sources/MapboxMaps/Style/StyleSourceManager.swift b/Sources/MapboxMaps/Style/StyleSourceManager.swift index 0727ce8ce337..5e030d4560f7 100644 --- a/Sources/MapboxMaps/Style/StyleSourceManager.swift +++ b/Sources/MapboxMaps/Style/StyleSourceManager.swift @@ -107,6 +107,7 @@ internal final class StyleSourceManager: StyleSourceManagerProtocol { } func addGeoJSONSourceFeatures(forSourceId sourceId: String, features: [Feature], dataId: String?) { + let identifier = UUID() let item = DispatchWorkItem { [weak self] in guard let self else { return } do { @@ -119,36 +120,51 @@ internal final class StyleSourceManager: StyleSourceManagerProtocol { Log.error(forMessage: "Failed to add features for source with id: \(sourceId), dataId: \(dataId ?? ""), error: \(error)") } } - workItemTracker.add(AnyCancelable(item.cancel), for: sourceId) + item.notify(queue: .main) { [weak self] in + self?.workItemTracker.removePartialUpdateCancelable(for: sourceId, uuid: identifier) + } + workItemTracker.addPartialUpdateCancelable(AnyCancelable(item.cancel), for: sourceId, uuid: identifier) backgroundQueue.async(execute: item) } private final class WorkItemPerGeoJSONSourceTracker { - private var cancellables = [SourceId: CompositeCancelable]() + private var cancelables = [SourceId: CompositeCancelable]() + private var partialUpdateCancelables = [SourceId: [UUID: Cancelable]]() - func add(_ cancellable: Cancelable, for sourceId: SourceId) { - let compositeCancellable: CompositeCancelable + func add(_ cancelable: Cancelable, for sourceId: SourceId) { + let compositeCancelable = cancelables[sourceId, default: .init()] - if let cached = cancellables[sourceId] { - compositeCancellable = cached - } else { - compositeCancellable = CompositeCancelable() - } + compositeCancelable.add(cancelable) + cancelables[sourceId] = compositeCancelable + } - compositeCancellable.add(cancellable) - cancellables[sourceId] = compositeCancellable + func addPartialUpdateCancelable(_ cancelable: Cancelable, for sourceId: SourceId, uuid: UUID) { + var sourceCancelables = partialUpdateCancelables[sourceId, default: .init()] + + sourceCancelables[uuid] = cancelable + partialUpdateCancelables[sourceId] = sourceCancelables + } + + func removePartialUpdateCancelable(for sourceId: SourceId, uuid: UUID) { + var sourceCancelables = partialUpdateCancelables[sourceId, default: .init()] + + sourceCancelables.removeValue(forKey: uuid) + partialUpdateCancelables[sourceId] = sourceCancelables } func cancelAll(for sourceId: SourceId) { - cancellables.removeValue(forKey: sourceId)?.cancel() + cancelables.removeValue(forKey: sourceId)?.cancel() + partialUpdateCancelables.removeValue(forKey: sourceId)?.values.forEach { $0.cancel() } } deinit { - cancellables.values.forEach { $0.cancel() } + cancelables.values.forEach { $0.cancel() } + partialUpdateCancelables.values.forEach { $0.values.forEach { $0.cancel() } } } } func updateGeoJSONSourceFeatures(forSourceId sourceId: String, features: [Feature], dataId: String?) { + let identifier = UUID() let item = DispatchWorkItem { [weak self] in guard let self else { return } do { @@ -161,12 +177,16 @@ internal final class StyleSourceManager: StyleSourceManagerProtocol { Log.error(forMessage: "Failed to update features for source with id: \(sourceId), dataId: \(dataId ?? ""), error: \(error)") } } + item.notify(queue: .main) { [weak self] in + self?.workItemTracker.removePartialUpdateCancelable(for: sourceId, uuid: identifier) + } - workItemTracker.add(AnyCancelable(item.cancel), for: sourceId) + workItemTracker.addPartialUpdateCancelable(AnyCancelable(item.cancel), for: sourceId, uuid: identifier) backgroundQueue.async(execute: item) } func removeGeoJSONSourceFeatures(forSourceId sourceId: String, featureIds: [String], dataId: String?) { + let identifier = UUID() let item = DispatchWorkItem { [weak self] in guard let self else { return } do { @@ -179,8 +199,11 @@ internal final class StyleSourceManager: StyleSourceManagerProtocol { Log.error(forMessage: "Failed to remove features for source with id: \(sourceId), dataId: \(dataId ?? ""), error: \(error)") } } + item.notify(queue: .main) { [weak self] in + self?.workItemTracker.removePartialUpdateCancelable(for: sourceId, uuid: identifier) + } - workItemTracker.add(AnyCancelable(item.cancel), for: sourceId) + workItemTracker.addPartialUpdateCancelable(AnyCancelable(item.cancel), for: sourceId, uuid: identifier) backgroundQueue.async(execute: item) } diff --git a/Tests/MapboxMapsTests/Foundation/Style/StyleSourceManagerTests.swift b/Tests/MapboxMapsTests/Foundation/Style/StyleSourceManagerTests.swift index 7b52d851421b..27f7f07c8820 100644 --- a/Tests/MapboxMapsTests/Foundation/Style/StyleSourceManagerTests.swift +++ b/Tests/MapboxMapsTests/Foundation/Style/StyleSourceManagerTests.swift @@ -558,6 +558,31 @@ final class StyleSourceManagerTests: XCTestCase { XCTAssertEqual(parameters.featureIds, featureIdentifiers) XCTAssertEqual(parameters.dataId, dataId) } + + func testPartialUpdateCancellableIsDeallocatedWhenUpdateIsComplete() { + let workItems = NSHashTable.weakObjects() + + autoreleasepool { + let dummyQueue = DispatchQueue(label: "Dummy queue", qos: .userInitiated) + let sourceId = "sourceId" + + sourceManager.addGeoJSONSourceFeatures(forSourceId: sourceId, features: [], dataId: nil) + sourceManager.updateGeoJSONSourceFeatures(forSourceId: sourceId, features: [], dataId: nil) + sourceManager.removeGeoJSONSourceFeatures(forSourceId: sourceId, featureIds: [], dataId: nil) + + backgroundQueue.asyncWorkItemStub.invocations.map(\.parameters).forEach(workItems.add) + backgroundQueue.asyncWorkItemStub.reset() + + XCTAssertEqual(workItems.count, 3) + + workItems.allObjects.forEach(dummyQueue.sync) + } + expectation(for: NSPredicate(block: { (_, _) in + workItems.anyObject == nil + }), evaluatedWith: nil) + + waitForExpectations(timeout: 3) + } } private extension GeoJSONSourceData {