Skip to content
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

Jump to bottom

feat: added a new CLI arg --merge-async to asynchronously and incrementally merge process coverage files to avoid OOM due to heap exhaustion #469

Merged
merged 5 commits into from May 26, 2023
Merged

feat: added a new CLI arg --merge-async to asynchronously and incrementally merge process coverage files to avoid OOM due to heap exhaustion #469

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5d56252
fix: updated getMergedProcessCov to be async and incrementally merge …
bizob2828 May 3, 2023
0978bdf
feat: added a new CLI arg `--merge-async` to asynchronously and incre…
bizob2828 May 23, 2023
028120e
chore: removed fixture and tests for unsupported versions of c8 < 10
bizob2828 May 23, 2023
5b65511
test: updated non-atomic test to clean up beforehand to work when run…
bizob2828 May 24, 2023
18e037a
chore: Enabled clean=true on the cobertura test to avoid issues when
bizob2828 May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
68 changes: 35 additions & 33 deletions lib/report.js
View file
Open in desktop
Expand Up @@ -2,7 +2,13 @@ const Exclude = require('test-exclude')
const libCoverage = require('istanbul-lib-coverage')
const libReport = require('istanbul-lib-report')
const reports = require('istanbul-reports')
const { readdirSync, readFileSync, statSync } = require('fs')
let readFile
try {
;({ readFile } = require('fs/promises'))
} catch (err) {
;({ readFile } = require('fs').promises)
}
const { readdirSync, statSync } = require('fs')
const { isAbsolute, resolve, extname } = require('path')
const { pathToFileURL, fileURLToPath } = require('url')
const getSourceMapFromFile = require('./source-map-from-file')
Expand Down Expand Up @@ -90,7 +96,7 @@ class Report {
if (this._allCoverageFiles) return this._allCoverageFiles

const map = libCoverage.createCoverageMap()
const v8ProcessCov = this._getMergedProcessCov()
const v8ProcessCov = await this._getMergedProcessCov()
const resultCountPerPath = new Map()
const possibleCjsEsmBridges = new Map()

Expand Down Expand Up @@ -174,24 +180,39 @@ class Report {
* @return {ProcessCov} Merged V8 process coverage.
* @private
*/
_getMergedProcessCov () {
async _getMergedProcessCov () {
const { mergeProcessCovs } = require('@bcoe/v8-coverage')
const v8ProcessCovs = []
const fileIndex = new Set() // Set<string>
for (const v8ProcessCov of this._loadReports()) {
if (this._isCoverageObject(v8ProcessCov)) {
if (v8ProcessCov['source-map-cache']) {
Object.assign(this.sourceMapCache, this._normalizeSourceMapCache(v8ProcessCov['source-map-cache']))
let mergedCov = null
for (const file of readdirSync(this.tempDirectory)) {
try {
const rawFile = await readFile(
resolve(this.tempDirectory, file),
'utf8'
)
let report = JSON.parse(rawFile)

if (this._isCoverageObject(report)) {
if (report['source-map-cache']) {
Object.assign(this.sourceMapCache, this._normalizeSourceMapCache(report['source-map-cache']))
}
report = this._normalizeProcessCov(report, fileIndex)
if (mergedCov) {
mergedCov = mergeProcessCovs([mergedCov, report])
} else {
mergedCov = mergeProcessCovs([report])
}
}
v8ProcessCovs.push(this._normalizeProcessCov(v8ProcessCov, fileIndex))
} catch (err) {
debuglog(`${err.stack}`)
}
}

if (this.all) {
const emptyReports = []
v8ProcessCovs.unshift({
const emptyReport = {
result: emptyReports
})
}
const workingDirs = this.src
const { extension } = this.exclude
for (const workingDir of workingDirs) {
Expand Down Expand Up @@ -222,9 +243,11 @@ class Report {
}
})
}

mergedCov = mergeProcessCovs([emptyReport, mergedCov])
}

return mergeProcessCovs(v8ProcessCovs)
return mergedCov
}

/**
Expand All @@ -237,27 +260,6 @@ class Report {
return maybeV8ProcessCov && Array.isArray(maybeV8ProcessCov.result)
}

/**
* Returns the list of V8 process coverages generated by Node.
*
* @return {ProcessCov[]} Process coverages generated by Node.
* @private
*/
_loadReports () {
Copy link
Contributor

@AriPerkkio AriPerkkio May 4, 2023 •
edited

Choose a reason for hiding this comment

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

Any chance this _loadReports could be kept here, and its API as is? This is something Vitest uses internally. https://github.com/vitest-dev/vitest/blob/708b10fe227c04b4219a1e5d4b539def009729b4/packages/coverage-c8/src/provider.ts#L94-L95

I've been meaning to open a PR here and propose this method as part of public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm ok I can keep loadReports but this PR fundamentally changes this in that it does not load all reports at once because of the OOM issues.

Copy link
Contributor Author

@bizob2828 bizob2828 May 4, 2023 •
edited

Choose a reason for hiding this comment

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

I think maybe I can provide this async merging as a CLI arg and keep existing functionality but that means for libraries that overrode "private" methods you'd have another one to override. @bcoe what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to prevent these OOMs would be to make _loadReports a bit more efficient:

  1. Load a single V8 report from file system and convert it to JSON
  2. Filter its result based on testExclude.shouldInstrument(result[].url) and filter out all unrelated coverage reports
  3. Return these filtered ScriptCoverage[] from _loadReport - no other changes required

The V8 reports collected using process.env.NODE_V8_COVERAGE contain so much noise as they include all node_modules and NodeJS internal node:<module-name> modules. When all V8 reports are loaded into memory at once, there is a lot unnecessary memory occupied by these irrelevant coverage reports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm. I could look into that. I realized I requested a review from you but I really would like @bcoe to take a look at this. Is this project still alive? there's quite a lot of PRs in the queue

const reports = []
for (const file of readdirSync(this.tempDirectory)) {
try {
reports.push(JSON.parse(readFileSync(
resolve(this.tempDirectory, file),
'utf8'
)))
} catch (err) {
debuglog(`${err.stack}`)
}
}
return reports
}

/**
* Normalizes a process coverage.
*
Expand Down
4 changes: 2 additions & 2 deletions test/integration.js.snap
View file
Open in desktop
Expand Up @@ -156,7 +156,7 @@ hey
---------------------------------------|---------|----------|---------|---------|-------------------
File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
---------------------------------------|---------|----------|---------|---------|-------------------
All files | 1.91 | 12 | 6.25 | 1.91 |
All files | 1.9 | 12 | 6.25 | 1.9 |
c8 | 0 | 0 | 0 | 0 |
index.js | 0 | 0 | 0 | 0 | 1
c8/bin | 0 | 0 | 0 | 0 |
Expand All @@ -168,7 +168,7 @@ All files | 1.91 | 12 | 6.25 | 1.91
c8/lib | 0 | 0 | 0 | 0 |
is-cjs-esm-bridge.js | 0 | 0 | 0 | 0 | 1-10
parse-args.js | 0 | 0 | 0 | 0 | 1-218
report.js | 0 | 0 | 0 | 0 | 1-340
report.js | 0 | 0 | 0 | 0 | 1-342
source-map-from-file.js | 0 | 0 | 0 | 0 | 1-100
c8/lib/commands | 0 | 0 | 0 | 0 |
check-coverage.js | 0 | 0 | 0 | 0 | 1-70
Expand Down