From be435b221b9b7551332b686693f31f6d6ef96230 Mon Sep 17 00:00:00 2001 From: Frank Viernau Date: Mon, 10 Feb 2025 09:03:02 +0100 Subject: [PATCH] fix(spdx-utils): Fix performance issue in callers of `and()` There are multiple code locations which use `reduce` together with `and()` to concatenate a given collection of expressions to a compound SPDX expression. If `n` expressions are given, then `n - 1` SPDX compound expressions instances get constructed via `n - 1` `and()` calls. As of [1] each `and()` execution started to invoke `equals()` at the very beginning. Dependening on the expression this can be very expensive. For example if the expression contains OR operators and is bit larger, then the call tree becomes quite heavy-weight. It's comprised of recursive `validChoices()` calls which insert expressions into sets which in turn leads to further `equals()` calls and so forth. After upgrading ORT from version 28.0.0 to 45.0.0 the execution of the evaluator took more than 3 days finishing, and the reporter took about 3 hours to finish for some real world scan. That issue has been introduced by [1], because reverting [1] (on top of `main`) does fix the performance problem. Reduce the mentioned `n - 1` `and()` calls to just a single one to relax the issue. This makes the evaluator finish in 1.8 seconds and the reporter in 3 seconds again, for the real world scan mentioned above. Fixes: #9902. [1]: 31b9be83c5793954a7d052a0f1e7e3ca4d9384bd Signed-off-by: Frank Viernau --- .../commands/GetPackageLicensesCommand.kt | 8 ++---- model/src/main/kotlin/PackageCurationData.kt | 3 ++- .../kotlin/licenses/ResolvedLicenseInfo.kt | 3 ++- .../src/main/kotlin/OpossumReporter.kt | 4 +-- .../spdx/src/main/kotlin/Extensions.kt | 22 +++++---------- .../src/main/kotlin/ScanOssResultParser.kt | 4 +-- utils/spdx/src/main/kotlin/SpdxExpression.kt | 27 ++++++++++++++----- 7 files changed, 38 insertions(+), 33 deletions(-) diff --git a/helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt b/helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt index e0ccc4a2b1bd4..cf548adb35901 100644 --- a/helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt +++ b/helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt @@ -41,7 +41,7 @@ import org.ossreviewtoolkit.utils.common.expandTilde import org.ossreviewtoolkit.utils.ort.ORT_CONFIG_FILENAME import org.ossreviewtoolkit.utils.ort.ortConfigDirectory import org.ossreviewtoolkit.utils.spdx.SpdxConstants -import org.ossreviewtoolkit.utils.spdx.SpdxExpression +import org.ossreviewtoolkit.utils.spdx.andOrNull internal class GetPackageLicensesCommand : OrtHelperCommand( help = "Shows the root license and the detected license for a package denoted by the given package identifier." @@ -113,11 +113,7 @@ internal class GetPackageLicensesCommand : OrtHelperCommand( } private fun Collection.toSpdxExpression(): String = - if (isEmpty()) { - SpdxConstants.NONE - } else { - asSequence().map { it.license }.distinct().reduce(SpdxExpression::and).sorted().toString() - } + map { it.license }.andOrNull()?.sorted()?.toString() ?: SpdxConstants.NONE private data class Result( val detectedLicense: String, diff --git a/model/src/main/kotlin/PackageCurationData.kt b/model/src/main/kotlin/PackageCurationData.kt index 6015e97367d49..c0557a44d5901 100644 --- a/model/src/main/kotlin/PackageCurationData.kt +++ b/model/src/main/kotlin/PackageCurationData.kt @@ -26,6 +26,7 @@ import org.ossreviewtoolkit.utils.common.zip import org.ossreviewtoolkit.utils.ort.DeclaredLicenseProcessor import org.ossreviewtoolkit.utils.spdx.SpdxExpression import org.ossreviewtoolkit.utils.spdx.SpdxExpression.Strictness.ALLOW_LICENSEREF_EXCEPTIONS +import org.ossreviewtoolkit.utils.spdx.andOrNull /** * This class contains curation data for a package. It is used to amend the automatically detected metadata for a @@ -183,7 +184,7 @@ data class PackageCurationData( purl = purl ?: other.purl, cpe = cpe ?: other.cpe, authors = authors.orEmpty() + other.authors.orEmpty(), - concludedLicense = setOfNotNull(concludedLicense, other.concludedLicense).reduce(SpdxExpression::and), + concludedLicense = setOfNotNull(concludedLicense, other.concludedLicense).andOrNull(), description = description ?: other.description, homepageUrl = homepageUrl ?: other.homepageUrl, binaryArtifact = binaryArtifact ?: other.binaryArtifact, diff --git a/model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt b/model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt index 978994f2e5c21..a572ab8c6b6a0 100644 --- a/model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt +++ b/model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt @@ -29,6 +29,7 @@ import org.ossreviewtoolkit.utils.ort.CopyrightStatementsProcessor import org.ossreviewtoolkit.utils.spdx.SpdxExpression import org.ossreviewtoolkit.utils.spdx.SpdxLicenseChoice import org.ossreviewtoolkit.utils.spdx.SpdxSingleLicenseExpression +import org.ossreviewtoolkit.utils.spdx.andOrNull /** * Resolved license information about a package (or project). @@ -70,7 +71,7 @@ data class ResolvedLicenseInfo( fun toCompoundExpression(): SpdxExpression? = licenses.flatMapTo(mutableSetOf()) { resolvedLicense -> resolvedLicense.originalExpressions.map { it.expression } - }.reduceOrNull(SpdxExpression::and) + }.andOrNull() /** * Return the effective [SpdxExpression] of this [ResolvedLicenseInfo] based on their [licenses] filtered by the diff --git a/plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt b/plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt index eeaf66d30e45e..a4552ed59fbaf 100644 --- a/plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt +++ b/plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt @@ -47,6 +47,7 @@ import org.ossreviewtoolkit.reporter.ReporterInput import org.ossreviewtoolkit.utils.common.packZip import org.ossreviewtoolkit.utils.ort.createOrtTempDir import org.ossreviewtoolkit.utils.spdx.SpdxLicense +import org.ossreviewtoolkit.utils.spdx.andOrNull private const val ISSUE_PRIORITY = 900 @@ -355,8 +356,7 @@ class OpossumReporter( val license = licenseFindings .filter { it.location.path == pathFromFinding } .map { it.license } - .distinct() - .reduceRightOrNull { left, right -> left and right } + .andOrNull() val pathSignal = OpossumSignal.create( source, diff --git a/plugins/reporters/spdx/src/main/kotlin/Extensions.kt b/plugins/reporters/spdx/src/main/kotlin/Extensions.kt index 81678a89288f3..3addaeb2b6ccf 100644 --- a/plugins/reporters/spdx/src/main/kotlin/Extensions.kt +++ b/plugins/reporters/spdx/src/main/kotlin/Extensions.kt @@ -49,6 +49,7 @@ import org.ossreviewtoolkit.utils.common.replaceCredentialsInUri import org.ossreviewtoolkit.utils.spdx.SpdxConstants import org.ossreviewtoolkit.utils.spdx.SpdxExpression import org.ossreviewtoolkit.utils.spdx.SpdxLicense +import org.ossreviewtoolkit.utils.spdx.andOrNull import org.ossreviewtoolkit.utils.spdx.calculatePackageVerificationCode import org.ossreviewtoolkit.utils.spdx.model.SpdxChecksum import org.ossreviewtoolkit.utils.spdx.model.SpdxDocument @@ -203,21 +204,12 @@ internal fun Package.toSpdxPackage( SpdxPackageType.PROJECT -> concludedLicense.nullOrBlankToSpdxNoassertionOrNone() else -> concludedLicense.nullOrBlankToSpdxNoassertionOrNone() }, - licenseDeclared = if (packageLicenseExpressions.isEmpty()) { - SpdxConstants.NONE - } else { - packageLicenseExpressions.reduce(SpdxExpression::and) - .simplify() - .sorted() - .nullOrBlankToSpdxNoassertionOrNone() - }, - licenseInfoFromFiles = if (packageVerificationCode == null) { - emptyList() - } else { - resolvedLicenseExpressions.filter(LicenseView.ONLY_DETECTED) - .mapTo(mutableSetOf()) { it.license.nullOrBlankToSpdxNoassertionOrNone() } - .sorted() - }, + licenseDeclared = packageLicenseExpressions + .andOrNull() + ?.simplify() + ?.sorted() + ?.nullOrBlankToSpdxNoassertionOrNone() + ?: SpdxConstants.NONE, name = id.name, originator = authors.takeUnless { it.isEmpty() }?.joinToString(prefix = "${SpdxConstants.PERSON} "), packageVerificationCode = packageVerificationCode, diff --git a/plugins/scanners/scanoss/src/main/kotlin/ScanOssResultParser.kt b/plugins/scanners/scanoss/src/main/kotlin/ScanOssResultParser.kt index 9258f8f91ca3b..689c0b33d6188 100644 --- a/plugins/scanners/scanoss/src/main/kotlin/ScanOssResultParser.kt +++ b/plugins/scanners/scanoss/src/main/kotlin/ScanOssResultParser.kt @@ -36,6 +36,7 @@ import org.ossreviewtoolkit.model.TextLocation import org.ossreviewtoolkit.utils.spdx.SpdxConstants import org.ossreviewtoolkit.utils.spdx.SpdxExpression import org.ossreviewtoolkit.utils.spdx.SpdxLicenseIdExpression +import org.ossreviewtoolkit.utils.spdx.andOrNull /** * Generate a summary from the given SCANOSS [result], using [startTime], [endTime] as metadata. This variant can be @@ -156,8 +157,7 @@ private fun getSnippets(details: ScanFileDetails): Set { return buildSet { purls.forEach { purl -> locations.forEach { snippetLocation -> - val license = licenses.reduceOrNull(SpdxExpression::and)?.sorted() - ?: SpdxLicenseIdExpression(SpdxConstants.NOASSERTION) + val license = licenses.andOrNull()?.sorted() ?: SpdxLicenseIdExpression(SpdxConstants.NOASSERTION) add(Snippet(score, snippetLocation, provenance, purl, license)) } diff --git a/utils/spdx/src/main/kotlin/SpdxExpression.kt b/utils/spdx/src/main/kotlin/SpdxExpression.kt index 9598150720ab3..4685ad3de47ff 100644 --- a/utils/spdx/src/main/kotlin/SpdxExpression.kt +++ b/utils/spdx/src/main/kotlin/SpdxExpression.kt @@ -291,12 +291,7 @@ class SpdxCompoundExpression( return children.sortedBy { it.toString() } } - return getSortedChildrenWithSameOperator(this).reduce( - when (operator) { - SpdxOperator.AND -> SpdxExpression::and - SpdxOperator.OR -> SpdxExpression::or - } - ) + return getSortedChildrenWithSameOperator(this).concat(operator) } override fun validate(strictness: Strictness) { @@ -638,3 +633,23 @@ data class SpdxLicenseReferenceExpression( override fun getLicenseUrl(): String? = null } + +fun Collection.and(): SpdxExpression = concat(SpdxOperator.AND) + +fun Collection.andOrNull(): SpdxExpression? = concatOrNull(SpdxOperator.AND) + +fun Collection.or(): SpdxExpression = concat(SpdxOperator.OR) + +fun Collection.orOrNull(): SpdxExpression? = concatOrNull(SpdxOperator.OR) + +fun Collection.concat(operator: SpdxOperator): SpdxExpression { + require(isNotEmpty()) { + "Cannot create a concatenated SPDX expression from an empty collection." + } + + val distinctExpressions = distinct() + return distinctExpressions.singleOrNull() ?: SpdxCompoundExpression(operator, distinctExpressions) +} + +fun Collection.concatOrNull(operator: SpdxOperator): SpdxExpression? = + takeIf { it.isNotEmpty() }?.let { concat(operator) }