From da46fbb762124727c6365a322eca10f35590bcc5 Mon Sep 17 00:00:00 2001 From: tomer doron Date: Fri, 11 Jan 2019 10:50:07 -0800 Subject: [PATCH] better caching motivation: better metrics cache, seperate mutex per type changes: * seperate mutex per metric type * more tests --- Sources/CoreMetrics/Metrics.swift | 47 ++++++++++-------- Tests/MetricsTests/CoreMetricsTests.swift | 58 +++++++++++++++++++---- Tests/MetricsTests/TestMetrics.swift | 29 ++++-------- 3 files changed, 87 insertions(+), 47 deletions(-) diff --git a/Sources/CoreMetrics/Metrics.swift b/Sources/CoreMetrics/Metrics.swift index 3462cba..cf2634d 100644 --- a/Sources/CoreMetrics/Metrics.swift +++ b/Sources/CoreMetrics/Metrics.swift @@ -120,10 +120,9 @@ public enum Metrics { private final class CachingMetricsHandler: MetricsHandler { private let wrapped: MetricsHandler - private let lock = Lock() // TODO: consider lock per cache? - private var counters = [String: Counter]() - private var Recorders = [String: Recorder]() - private var timers = [String: Timer]() + private var counters = Cache() + private var recorders = Cache() + private var timers = Cache() public static func wrap(_ handler: MetricsHandler) -> CachingMetricsHandler { if let caching = handler as? CachingMetricsHandler { @@ -138,35 +137,43 @@ private final class CachingMetricsHandler: MetricsHandler { } public func makeCounter(label: String, dimensions: [(String, String)]) -> Counter { - return self.make(label: label, dimensions: dimensions, cache: &self.counters, maker: self.wrapped.makeCounter) + return counters.getOrSet(label: label, dimensions:dimensions, maker: self.wrapped.makeCounter) } public func makeRecorder(label: String, dimensions: [(String, String)], aggregate: Bool) -> Recorder { let maker = { (label: String, dimensions: [(String, String)]) -> Recorder in self.wrapped.makeRecorder(label: label, dimensions: dimensions, aggregate: aggregate) } - return self.make(label: label, dimensions: dimensions, cache: &self.Recorders, maker: maker) + return recorders.getOrSet(label: label, dimensions:dimensions, maker: maker) } public func makeTimer(label: String, dimensions: [(String, String)]) -> Timer { - return self.make(label: label, dimensions: dimensions, cache: &self.timers, maker: self.wrapped.makeTimer) + return timers.getOrSet(label: label, dimensions:dimensions, maker: self.wrapped.makeTimer) } - private func make(label: String, dimensions: [(String, String)], cache: inout [String: Item], maker: (String, [(String, String)]) -> Item) -> Item { - let fqn = self.fqn(label: label, dimensions: dimensions) - return self.lock.withLock { - if let item = cache[fqn] { - return item - } else { - let item = maker(label, dimensions) - cache[fqn] = item - return item + private class Cache { + private var items = [String: T]() + // using a mutex is never ideal, we will need to explore optimization options + // once we see how real life workloads behaves + // for example, for short opetations like hashmap lookup mutexes are worst than r/w locks in 99% reads, but better than them in mixed r/w mode + private let lock = Lock() + + func getOrSet(label: String, dimensions: [(String, String)], maker: (String, [(String, String)]) -> T) -> T { + let key = self.fqn(label: label, dimensions: dimensions) + return self.lock.withLock { + if let item = items[key] { + return item + } else { + let item = maker(label, dimensions) + items[key] = item + return item + } } } - } - - private func fqn(label: String, dimensions: [(String, String)]) -> String { - return [[label], dimensions.compactMap { $0.1 }].flatMap { $0 }.joined(separator: ".") + + private func fqn(label: String, dimensions: [(String, String)]) -> String { + return [[label], dimensions.compactMap { "\($0.0).\($0.1)" }].flatMap { $0 }.joined(separator: ".") + } } } diff --git a/Tests/MetricsTests/CoreMetricsTests.swift b/Tests/MetricsTests/CoreMetricsTests.swift index c548c23..bc0b60d 100644 --- a/Tests/MetricsTests/CoreMetricsTests.swift +++ b/Tests/MetricsTests/CoreMetricsTests.swift @@ -194,13 +194,33 @@ class MetricsTests: XCTestCase { counter.increment(value) } handlers.forEach { handler in - let counter = handler.counters[name] as! TestCounter + let counter = handler.counters[0] as! TestCounter + XCTAssertEqual(counter.label, name, "expected label to match") XCTAssertEqual(counter.values.count, 1, "expected number of entries to match") XCTAssertEqual(counter.values[0].1, Int64(value), "expected value to match") } } - func testDimensions() throws { + func testCaching() throws { + // bootstrap with our test metrics + let metrics = TestMetrics() + Metrics.bootstrap(metrics) + // run the test + let name = "counter-\(NSUUID().uuidString)" + let counter = Metrics.global.makeCounter(label: name) as! TestCounter + // same + let name2 = name + let counter2 = Metrics.global.makeCounter(label: name2) as! TestCounter + XCTAssertEqual(counter2.label, name2, "expected label to match") + XCTAssertEqual(counter2, counter, "expected caching to work with dimensions") + // different name + let name3 = "counter-\(NSUUID().uuidString)" + let counter3 = Metrics.global.makeCounter(label: name3) as! TestCounter + XCTAssertEqual(counter3.label, name3, "expected label to match") + XCTAssertNotEqual(counter3, counter, "expected caching to work with dimensions") + } + + func testCachingWithDimensions() throws { // bootstrap with our test metrics let metrics = TestMetrics() Metrics.bootstrap(metrics) @@ -208,13 +228,35 @@ class MetricsTests: XCTestCase { let name = "counter-\(NSUUID().uuidString)" let dimensions = [("foo", "bar"), ("baz", "quk")] let counter = Metrics.global.makeCounter(label: name, dimensions: dimensions) as! TestCounter - counter.increment() - - XCTAssertEqual(counter.values.count, 1, "expected number of entries to match") - XCTAssertEqual(counter.values[0].1, 1, "expected value to match") + XCTAssertEqual(counter.label, name, "expected dimensions to match") XCTAssertEqual(counter.dimensions.description, dimensions.description, "expected dimensions to match") - - let counter2 = Metrics.global.makeCounter(label: name, dimensions: dimensions) as! TestCounter + // same + let name2 = name + let dimensions2 = dimensions + let counter2 = Metrics.global.makeCounter(label: name2, dimensions: dimensions2) as! TestCounter + XCTAssertEqual(counter2.label, name2, "expected label to match") + XCTAssertEqual(counter2.dimensions.description, dimensions2.description, "expected dimensions to match") XCTAssertEqual(counter2, counter, "expected caching to work with dimensions") + // different name + let name3 = "counter-\(NSUUID().uuidString)" + let dimensions3 = dimensions + let counter3 = Metrics.global.makeCounter(label: name3, dimensions: dimensions3) as! TestCounter + XCTAssertEqual(counter3.label, name3, "expected label to match") + XCTAssertEqual(counter3.dimensions.description, dimensions3.description, "expected dimensions to match") + XCTAssertNotEqual(counter3, counter, "expected caching to work with dimensions") + // different dimensions "key" + let name4 = name + let dimensions4 = dimensions.map{ ($0.0 + "-test" , $0.1) } + let counter4 = Metrics.global.makeCounter(label: name4, dimensions: dimensions4) as! TestCounter + XCTAssertEqual(counter4.label, name4, "expected label to match") + XCTAssertEqual(counter4.dimensions.description, dimensions4.description, "expected dimensions to match") + XCTAssertNotEqual(counter4, counter, "expected caching to work with dimensions") + // different dimensions "value" + let name5 = name + let dimensions5 = dimensions.map{ ($0.0, $0.1 + "-test") } + let counter5 = Metrics.global.makeCounter(label: name5, dimensions: dimensions5) as! TestCounter + XCTAssertEqual(counter5.label, name5, "expected label to match") + XCTAssertEqual(counter5.dimensions.description, dimensions5.description, "expected dimensions to match") + XCTAssertNotEqual(counter5, counter, "expected caching to work with dimensions") } } diff --git a/Tests/MetricsTests/TestMetrics.swift b/Tests/MetricsTests/TestMetrics.swift index 1807758..39d8648 100644 --- a/Tests/MetricsTests/TestMetrics.swift +++ b/Tests/MetricsTests/TestMetrics.swift @@ -4,41 +4,32 @@ import Foundation internal class TestMetrics: MetricsHandler { private let lock = NSLock() // TODO: consider lock per cache? - var counters = [String: Counter]() - var recorders = [String: Recorder]() - var timers = [String: Timer]() + var counters = [Counter]() + var recorders = [Recorder]() + var timers = [Timer]() public func makeCounter(label: String, dimensions: [(String, String)]) -> Counter { - return self.make(label: label, dimensions: dimensions, cache: &self.counters, maker: TestCounter.init) + return self.make(label: label, dimensions: dimensions, registry: &self.counters, maker: TestCounter.init) } public func makeRecorder(label: String, dimensions: [(String, String)], aggregate: Bool) -> Recorder { let maker = { (label: String, dimensions: [(String, String)]) -> Recorder in TestRecorder(label: label, dimensions: dimensions, aggregate: aggregate) } - return self.make(label: label, dimensions: dimensions, cache: &self.recorders, maker: maker) + return self.make(label: label, dimensions: dimensions, registry: &self.recorders, maker: maker) } public func makeTimer(label: String, dimensions: [(String, String)]) -> Timer { - return self.make(label: label, dimensions: dimensions, cache: &self.timers, maker: TestTimer.init) + return self.make(label: label, dimensions: dimensions, registry: &self.timers, maker: TestTimer.init) } - private func make(label: String, dimensions: [(String, String)], cache: inout [String: Item], maker: (String, [(String, String)]) -> Item) -> Item { - let fqn = self.fqn(label: label, dimensions: dimensions) + private func make(label: String, dimensions: [(String, String)], registry: inout [Item], maker: (String, [(String, String)]) -> Item) -> Item { + let item = maker(label, dimensions) return self.lock.withLock { - if let item = cache[fqn] { - return item - } else { - let item = maker(label, dimensions) - cache[fqn] = item - return item - } + registry.append(item) + return item } } - - private func fqn(label: String, dimensions: [(String, String)]) -> String { - return [[label], dimensions.compactMap { $0.1 }].flatMap { $0 }.joined(separator: ".") - } } internal class TestCounter: Counter, Equatable {