From c8f3493c0bde7e7b8ba12696e545a19edf3358d2 Mon Sep 17 00:00:00 2001 From: Johannes Koenen Date: Tue, 2 Jan 2024 10:14:01 +0100 Subject: [PATCH] Review: Replace ArrayList removal with IdentitySet, iterate baseline only once --- .../qodana/sarif/baseline/Baseline.kt | 52 +++++++------------ .../sarif/baseline/BaselineCollections.kt | 44 ++++++++++++++++ .../qodana/sarif/baseline/MultiMap.kt | 14 ----- 3 files changed, 63 insertions(+), 47 deletions(-) create mode 100644 sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/BaselineCollections.kt delete mode 100644 sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/MultiMap.kt diff --git a/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/Baseline.kt b/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/Baseline.kt index de973de..2d6008f 100644 --- a/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/Baseline.kt +++ b/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/Baseline.kt @@ -7,20 +7,6 @@ import com.jetbrains.qodana.sarif.model.Run private typealias FingerprintIndex = MultiMap -private class Counter(private val underlying: MutableMap = mutableMapOf()) { - operator fun get(key: T) = underlying.getOrDefault(key, 0) - fun increment(key: T) = underlying.compute(key) { _, o -> (o ?: 0).inc() }!! - fun decrement(key: T) = underlying.compute(key) { _, o -> (o ?: 0).dec() }!! -} - -private inline fun MutableIterable.each(crossinline f: MutableIterator.(T) -> Unit) { - val iter = iterator() - // don't use `iter.forEachRemaining` as that is incompatible with `iter.remove` - while (iter.hasNext()) { - iter.f(iter.next()) - } -} - private fun Iterable?.noNulls(): Sequence = this?.asSequence().orEmpty().filterNotNull() @@ -54,41 +40,41 @@ internal class DiffState(private val options: Options) { /** CAUTION: This mutates results in report and baseline **/ internal fun applyBaseline(report: Run, baseline: Run, options: Options): DiffState { + val state = DiffState(options) + val reportDescriptors = DescriptorLookup(report) val baselineDescriptors = DescriptorLookup(baseline) val reportIndex = FingerprintIndex() val baselineCounter = Counter() - // shallow copies, to not mess with the underlying reports val undecidedFromReport = report.results.noNulls() .filterNot { it.baselineState == BaselineState.ABSENT } .onEach { result -> - result.equalIndicators.forEach { print -> reportIndex.add(print, result) } + result.equalIndicators + .forEach { print -> reportIndex.add(print, result) } } - .toMutableList() + .toCollection(IdentitySet(report.results?.size ?: 0)) val undecidedFromBaseline = baseline.results.noNulls() .filterNot { it.baselineState == BaselineState.ABSENT } .onEach { result -> baselineCounter.increment(ResultKey(result)) } - .toMutableList() - - val state = DiffState(options) - - undecidedFromBaseline.each { result -> - val foundInReport = result.equalIndicators - .flatMap(reportIndex::getOrEmpty) - .filter(undecidedFromReport::remove) - .onEach { state.put(it, BaselineState.UNCHANGED) } - .firstOrNull() != null - - when { - foundInReport -> remove() - !options.wasChecked.apply(result) -> { - remove() + .filter { result -> + val foundInReport = result.equalIndicators + .flatMap(reportIndex::getOrEmpty) + .filter(undecidedFromReport::remove) + .onEach { state.put(it, BaselineState.UNCHANGED) } + .firstOrNull() != null + + if (foundInReport) { + return@filter false + } + if (!options.wasChecked.apply(result)) { state.put(result, BaselineState.UNCHANGED) + return@filter false } + true } - } + .toMutableList() undecidedFromReport.forEach { result -> val key = ResultKey(result) diff --git a/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/BaselineCollections.kt b/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/BaselineCollections.kt new file mode 100644 index 0000000..8e9de9c --- /dev/null +++ b/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/BaselineCollections.kt @@ -0,0 +1,44 @@ +package com.jetbrains.qodana.sarif.baseline + +import java.util.* + +internal class MultiMap private constructor( + private val underlying: MutableMap> +) : Iterable>> { + constructor() : this(mutableMapOf()) + + fun add(key: K, value: V) { + underlying.compute(key) { _, old -> old?.apply { add(value) } ?: mutableListOf(value) } + } + + fun getOrEmpty(key: K): MutableList = underlying[key] ?: mutableListOf() + + override fun iterator(): Iterator>> = underlying.iterator() +} + +internal class IdentitySet private constructor( + private val map: IdentityHashMap +) : MutableSet by map.keys { + constructor(expectedSize: Int) : this(IdentityHashMap(expectedSize)) + + private var insertCounter = 0u + + override fun add(element: T): Boolean = map.putIfAbsent(element, insertCounter++) == null + override fun addAll(elements: Collection): Boolean = elements.fold(false) { r, e -> add(e) || r } + + override fun iterator(): MutableIterator = object : MutableIterator { + private val underlying = map.entries.sortedBy(MutableMap.MutableEntry::value).iterator() + + override fun hasNext(): Boolean = underlying.hasNext() + override fun next(): T = underlying.next().key + override fun remove() = throw UnsupportedOperationException("Cannot remove elements from this iterator") + } +} + +internal class Counter { + private val underlying: MutableMap = mutableMapOf() + + operator fun get(key: T) = underlying.getOrDefault(key, 0) + fun increment(key: T) = underlying.compute(key) { _, o -> (o ?: 0).inc() }!! + fun decrement(key: T) = underlying.compute(key) { _, o -> (o ?: 0).dec() }!! +} diff --git a/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/MultiMap.kt b/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/MultiMap.kt deleted file mode 100644 index b1a5408..0000000 --- a/sarif/src/main/java/com/jetbrains/qodana/sarif/baseline/MultiMap.kt +++ /dev/null @@ -1,14 +0,0 @@ -package com.jetbrains.qodana.sarif.baseline - -internal data class MultiMap @JvmOverloads constructor( - private val underlying: MutableMap> = mutableMapOf() -) : Iterable>> { - - fun add(key: K, value: V) { - underlying.compute(key) { _, old -> old?.apply { add(value) } ?: mutableListOf(value) } - } - - fun getOrEmpty(key: K): MutableList = underlying[key] ?: mutableListOf() - - override fun iterator(): Iterator>> = underlying.iterator() -}