Skip to content

feat(npm): Speed-up getting the remote package details #10059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 21, 2025
Merged

feat(npm): Speed-up getting the remote package details #10059

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions plugins/package-managers/node/src/main/kotlin/npm/Npm.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@
import java.io.File
import java.util.LinkedList

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.async
import kotlinx.coroutines.awaitAll
import kotlinx.coroutines.withContext

import org.apache.logging.log4j.kotlin.logger

import org.ossreviewtoolkit.analyzer.PackageManagerFactory
Expand All @@ -45,6 +50,7 @@
import org.ossreviewtoolkit.utils.common.ProcessCapture
import org.ossreviewtoolkit.utils.common.collectMessages
import org.ossreviewtoolkit.utils.common.withoutPrefix
import org.ossreviewtoolkit.utils.ort.runBlocking

import org.semver4j.RangesList
import org.semver4j.RangesListFactory
Expand Down Expand Up @@ -126,6 +132,9 @@
val project = parseProject(definitionFile, analysisRoot)
val projectModuleInfo = listModules(workingDir, issues).undoDeduplication()

// Warm-up the cache to speed-up processing.
requestAllPackageDetails(projectModuleInfo)

Check warning on line 136 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L136 

Added line #L136 was not covered by tests

val scopeNames = Scope.entries
.filterNot { excludes.isScopeExcluded(it.descriptor) }
.mapTo(mutableSetOf()) { scope ->
Expand Down Expand Up @@ -179,13 +188,41 @@

return process.extractNpmIssues()
}

private fun requestAllPackageDetails(projectModuleInfo: ModuleInfo) {
runBlocking {
withContext(Dispatchers.IO.limitedParallelism(20)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this special value ? Is it from empirical testing ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It aligns with other similar code that was introduced by @mnonnenmacher in 23c9bb0. But I agree that we should afterwards probably extract a constant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 23c9bb0 20 was chosen randomly, simply because I had not time to test which number provides the best performance in each of the places. I think adding a constant would give the false impression that there is any reason to use the same number in all of the places.

projectModuleInfo.getAllPackageNodeModuleIds().map { packageName ->

Check warning on line 195 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L193-L195 

Added lines #L193 - L195 were not covered by tests
async { getRemotePackageDetails(packageName) }
}.awaitAll()
}

Check warning on line 198 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L197-L198 

Added lines #L197 - L198 were not covered by tests
}
}

Check warning on line 200 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L200 

Added line #L200 was not covered by tests
}

private enum class Scope(val descriptor: String) {
DEPENDENCIES("dependencies"),
DEV_DEPENDENCIES("devDependencies")
}

private fun ModuleInfo.getAllPackageNodeModuleIds(): Set<String> {
val queue = Scope.entries.flatMapTo(LinkedList()) { getScopeDependencies(it) }
val result = mutableSetOf<String>()

Check warning on line 210 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L209-L210 

Added lines #L209 - L210 were not covered by tests

while (queue.isNotEmpty()) {
val info = queue.removeFirst()

Check warning on line 213 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L213 

Added line #L213 was not covered by tests

@Suppress("ComplexCondition")
if (!info.isProject && info.isInstalled && !info.name.isNullOrBlank() && !info.version.isNullOrBlank()) {
Copy link
Member

@sschuberth sschuberth Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity: Aren't isProject and isInstalled mutually exclusive? I.e. can a project ever get installed?

Copy link
Member Author

@fviernau fviernau Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInstalled refers to whether there is a packageJson file existing under Path.

The !isProject can be interpreted as isPackage and combining this with isInstalled to me makes a lot of sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isInstalled refers to whether there is a packageJson file existing under Path.

For the record, here's what I did to confirm that statement for my own confidence. Drilling down, isInstalled is implemented as

private val ModuleInfo.isInstalled: Boolean get() = path != null

and path is documented as

The path to the directory where the package is *installed*.

(emphasis mine, as that's the part that confused me). Also, ModuleInfo is populated from output of npm list, which is documented as "This command will print to stdout all the versions of packages that are installed". So to me that sounded like we'll only get data in ModuleInfo (and the path property) for things that have been installed before via npm install.

But the root project itself seems to be an exception to that. Running npm list --json --long e.g. in plugins/package-managers/node/src/funTest/assets/projects/synthetic/pnpm/nested-project creates JSON output that does contain a path for the root project's package.json. However, it does not contain a path for sub/package.json.

result += "${info.name}@${info.version}"

Check warning on line 217 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L217 

Added line #L217 was not covered by tests
}

Scope.entries.flatMapTo(queue) { info.getScopeDependencies(it) }

Check warning on line 220 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L220 

Added line #L220 was not covered by tests
}

return result

Check warning on line 223 in plugins/package-managers/node/src/main/kotlin/npm/Npm.kt

View check run for this annotation

Codecov / codecov/patch

plugins/package-managers/node/src/main/kotlin/npm/Npm.kt#L223 

Added line #L223 was not covered by tests
}

private fun ModuleInfo.getScopeDependencies(scope: Scope) =
when (scope) {
Scope.DEPENDENCIES -> dependencies.values.filter { !it.dev }
Expand Down
4 changes: 2 additions & 2 deletions plugins/package-managers/node/src/main/kotlin/npm/NpmDependencyHandler.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ internal class NpmDependencyHandler(
}
}

private val ModuleInfo.isInstalled: Boolean get() = path != null
internal val ModuleInfo.isInstalled: Boolean get() = path != null

private val ModuleInfo.isProject: Boolean get() = resolved == null
internal val ModuleInfo.isProject: Boolean get() = resolved == null

private val ModuleInfo.packageJsonFile: File get() =
File(
Expand Down
Loading