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
a 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 without 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 fbdc7fd
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 26 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
15 changes: 7 additions & 8 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,14 +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()
},
licenseDeclared = packageLicenseExpressions
.andOrNull()
?.simplify()
?.sorted()
?.nullOrBlankToSpdxNoassertionOrNone()
?: SpdxConstants.NONE,
licenseInfoFromFiles = if (packageVerificationCode == null) {
emptyList()
} else {
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 fbdc7fd

Please sign in to comment.