feat(npm): Speed-up getting the remote package details #10059
Conversation
fviernau
force-pushed
the
npm-view-concurrency
branch
from
c692187 to
adc2520
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10059 +/- ##
============================================
+ Coverage 69.57% 69.64% +0.07%
- Complexity 1462 1464 +2
============================================
Files 270 270
Lines 9665 9681 +16
Branches 1025 1028 +3
============================================
+ Hits 6724 6742 +18
+ Misses 2491 2489 -2
Partials 450 450
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| private fun requestAllPackageDetails(projectModuleInfo: ModuleInfo) { | ||
| runBlocking { | ||
| withContext(Dispatchers.IO.limitedParallelism(20)) { |
There was a problem hiding this comment.
Why this special value ? Is it from empirical testing ?
There was a problem hiding this comment.
It aligns with other similar code that was introduced by @mnonnenmacher in 23c9bb0. But I agree that we should afterwards probably extract a constant.
fviernau
force-pushed
the
npm-view-concurrency
branch
from
adc2520 to
2d457c9
Compare
Obtaining the package details via `npm info` is a performance bottleneck of ORT's NPM package manager. Request the package details for all packages upfront, in parallel to reduce execution time. Experiments on a development machine show that execution of `NpmFunTest` now takes `1 min 13 sec` instead of `3 min 47 sec`. Fixes: #9950. Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
npm-view-concurrency
branch
from
2d457c9 to
30bfebe
Compare
| val info = queue.removeFirst() | ||
|
|
||
| @Suppress("ComplexCondition") | ||
| if (!info.isProject && info.isInstalled && !info.name.isNullOrBlank() && !info.version.isNullOrBlank()) { |
There was a problem hiding this comment.
Out of curiosity: Aren't isProject and isInstalled mutually exclusive? I.e. can a project ever get installed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
isInstalledrefers to whether there is apackageJsonfile existing underPath.
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.
| } | ||
|
|
||
| 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>() |
There was a problem hiding this comment.
Nit: Could use a surrounding buildSet {} (and function expression).
There was a problem hiding this comment.
@fviernau, are you ok if I address this in a small follow-up?
There was a problem hiding this comment.
@fviernau, are you ok if I address this in a small follow-up?
sure.
Obtaining the package details via
npm infois a performance bottleneck of ORT's NPM package manager. Request the package details for all packages upfront, in parellel to reduce execution time. Experiments on a development machine show that execution ofNpmFunTestnow takes1 min 13 secinstead of3 min 47 sec.Fixes: #9950.
before
after