Skip to content

Commit

Permalink
fix(spdx-utils): Fix performance issue in callers of and()
Browse files Browse the repository at this point in the history
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]: 31b9be8

Signed-off-by: Frank Viernau <[email protected]>
  • Loading branch information
fviernau committed Feb 10, 2025
1 parent 6af84b1 commit be435b2
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 33 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand Down Expand Up @@ -113,11 +113,7 @@ internal class GetPackageLicensesCommand : OrtHelperCommand(
}

private fun Collection<LicenseFinding>.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,
Expand Down
3 changes: 2 additions & 1 deletion model/src/main/kotlin/PackageCurationData.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
3 changes: 2 additions & 1 deletion model/src/main/kotlin/licenses/ResolvedLicenseInfo.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
Expand Down
22 changes: 7 additions & 15 deletions plugins/reporters/spdx/src/main/kotlin/Extensions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -156,8 +157,7 @@ private fun getSnippets(details: ScanFileDetails): Set<Snippet> {
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))
}
Expand Down
27 changes: 21 additions & 6 deletions utils/spdx/src/main/kotlin/SpdxExpression.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -638,3 +633,23 @@ data class SpdxLicenseReferenceExpression(

override fun getLicenseUrl(): String? = null
}

fun Collection<SpdxExpression>.and(): SpdxExpression = concat(SpdxOperator.AND)

fun Collection<SpdxExpression>.andOrNull(): SpdxExpression? = concatOrNull(SpdxOperator.AND)

fun Collection<SpdxExpression>.or(): SpdxExpression = concat(SpdxOperator.OR)

fun Collection<SpdxExpression>.orOrNull(): SpdxExpression? = concatOrNull(SpdxOperator.OR)

Check warning on line 643 in utils/spdx/src/main/kotlin/SpdxExpression.kt

View workflow job for this annotation

GitHub Actions / qodana-scan

Unused symbol

Function "orOrNull" is never used

Check warning

Code scanning / QDJVMC

Unused symbol Warning

Function "orOrNull" is never used

fun Collection<SpdxExpression>.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<SpdxExpression>.concatOrNull(operator: SpdxOperator): SpdxExpression? =
takeIf { it.isNotEmpty() }?.let { concat(operator) }

0 comments on commit be435b2

Please sign in to comment.