Skip to content

Enhance reading & loading & rendering performance #503

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

Open
JackKuo-tw wants to merge 2 commits into
base: master
Choose a base branch
from
+119 −110
Open

Enhance reading & loading & rendering performance #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31467e8
perf: Optimize coverage reading and rendering performance
JackKuo-tw Jan 30, 2026
bc0c0d7
perf: optimize coverage loading and rendering performance
JackKuo-tw Jan 30, 2026
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
96 changes: 55 additions & 41 deletions src/coverage-system/renderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,12 @@ export interface ICoverageLines {
none: Range[];
}

interface CoverageSets {
full: Set<number>;
partial: Set<number>;
none: Set<number>;
}

export class Renderer {
private configStore: Config;
private sectionFinder: SectionFinder;
Expand All @@ -33,29 +39,31 @@ export class Renderer {
sections: Map<string, Section>,
textEditors: readonly TextEditor[],
) {
const coverageLines: ICoverageLines = {
full: [],
none: [],
partial: [],
};

// Single-pass iteration for better performance
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

I haven't done any performance tuning for the extension in a long while, I will have to refresh my memory on some of the debugging logs I have used in the past (what method did you use to judge the performance improvement?).

textEditors.forEach((textEditor) => {
// Remove all decorations first to prevent graphical issues
this.removeDecorationsForEditor(textEditor);
});

textEditors.forEach((textEditor) => {
// Reset lines for new editor
coverageLines.full = [];
coverageLines.none = [];
coverageLines.partial = [];
const coverageSets: CoverageSets = {
full: new Set<number>(),
none: new Set<number>(),
partial: new Set<number>(),
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

I guess it makes sense for these to be sets as you can only have one unique coverage type per line (there are scenarios where you can have multiple coverage types on the same line though).

I don't believe we have this ability yet (via the extension) but it might come in a future PR / ticket.
#497
https://llvm.org/docs/CoverageMappingFormat.html#mapping-region

};

// find the section(s) (or undefined) by looking relatively at each workspace
// users can also optional use absolute instead of relative for this
const foundSections = this.sectionFinder.findSectionsForEditor(textEditor, sections);
if (!foundSections.length) { return; }

this.filterCoverage(foundSections, coverageLines);
this.filterCoverage(foundSections, coverageSets);

const coverageLines: ICoverageLines = {
full: this.setsToRanges(coverageSets.full),
none: this.setsToRanges(coverageSets.none),
partial: this.setsToRanges(coverageSets.partial),
};

this.setDecorationsForEditor(textEditor, coverageLines);
});
}
Expand Down Expand Up @@ -97,60 +105,66 @@ export class Renderer {
/**
* Takes an array of sections and computes the coverage lines
* @param sections sections to filter the coverage for
* @param coverageLines the current coverage lines as this point in time
* @param coverageSets the current coverage sets as this point in time
*/
private filterCoverage(
sections: Section[],
coverageLines: ICoverageLines,
coverageSets: CoverageSets,
) {
sections.forEach((section) => {
this.filterLineCoverage(section, coverageLines);
this.filterBranchCoverage(section, coverageLines);
this.filterLineCoverage(section, coverageSets);
this.filterBranchCoverage(section, coverageSets);
});
}

private filterLineCoverage(
section: Section,
coverageLines: ICoverageLines,
coverageSets: CoverageSets,
) {
if (!section || !section.lines) {
return;
}
section.lines.details
.filter((detail) => detail.line > 0)
.forEach((detail) => {
const lineRange = new Range(detail.line - 1, 0, detail.line - 1, 0);
if (detail.hit > 0) {
// Evaluates to true if at least one element in range is equal to LineRange
if (coverageLines.none.some((range) => range.isEqual(lineRange))) {
coverageLines.none = coverageLines.none.filter((range) => !range.isEqual(lineRange))
}
coverageLines.full.push(lineRange);
} else {
if (!coverageLines.full.some((range) => range.isEqual(lineRange))) {
// only add a none coverage if no full ones exist
coverageLines.none.push(lineRange);
.filter((detail) => detail.line > 0)
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

Minor code styling: We probably want to leave this as is and use the code formatter we have set up in the pre-commit hooks. (If you are already using that then great, we can disregard my comment).

.forEach((detail) => {
const line = detail.line - 1;
if (detail.hit > 0) {
if (coverageSets.none.has(line)) {
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

We might want to add back the comment (or update it slightly) so we remember that this helps remove the none coverage if there is atleast some type of coverage hit.

coverageSets.none.delete(line);
}
coverageSets.full.add(line);
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

This cleaner section replaces the logic below, which looks to be better / equivalent in my eyes?

                // Evaluates to true if at least one element in range is equal to LineRange
                if (coverageLines.none.some((range) => range.isEqual(lineRange))) {
                    coverageLines.none = coverageLines.none.filter((range) => !range.isEqual(lineRange))
                }
                coverageLines.full.push(lineRange);

} else {
if (!coverageSets.full.has(line)) {
// only add a none coverage if no full ones exist
coverageSets.none.add(line);
}
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

Nice rework @JackKuo-tw (+Claude 🤖)

}
}
});
});
}

private filterBranchCoverage(
section: Section,
coverageLines: ICoverageLines,
coverageSets: CoverageSets,
) {
if (!section || !section.branches) {
return;
}
section.branches.details
.filter((detail) => detail.taken === 0 && detail.line > 0)
.forEach((detail) => {
const partialRange = new Range(detail.line - 1, 0, detail.line - 1, 0);
// Evaluates to true if at least one element in range is equal to partialRange
if (coverageLines.full.some((range) => range.isEqual(partialRange))){
coverageLines.full = coverageLines.full.filter((range) => !range.isEqual(partialRange));
coverageLines.partial.push(partialRange);
}
.filter((detail) => detail.taken === 0 && detail.line > 0)
.forEach((detail) => {
const line = detail.line - 1;
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

We probably want to add back the comment and update the variable to be about partial coverage again.

if (coverageSets.full.has(line)) {
coverageSets.full.delete(line);
coverageSets.partial.add(line);
}
});
}

private setsToRanges(lines: Set<number>): Range[] {
const ranges: Range[] = [];
lines.forEach((line) => {
ranges.push(new Range(line, 0, line, 0));
});
return ranges;
}
}
53 changes: 25 additions & 28 deletions src/files/coverageparser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,39 +21,42 @@ export class CoverageParser {
files: Map<string, string>
): Promise<Map<string, Section>> {
const coverages = new Map<string, Section>();
const parsePromises: Promise<Section[]>[] = [];

for (const [fileName, fileContent] of files) {
// get coverage file type
const coverageFile = new CoverageFile(fileContent);
switch (coverageFile.type) {
case CoverageType.CLOVER:
await this.xmlExtractClover(
coverages,
parsePromises.push(this.xmlExtractClover(
fileName,
fileContent
);
));
Comment on lines +31 to +34
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

Personal preference, but if the code linter / reformatter doesn't care then we can ignore this.

Suggested change
parsePromises.push(this.xmlExtractClover(
fileName,
fileContent
);
));
parsePromises.push(
this.xmlExtractClover(
fileName,
fileContent
)
);

break;
case CoverageType.JACOCO:
await this.xmlExtractJacoco(
coverages,
parsePromises.push(this.xmlExtractJacoco(
fileName,
fileContent
);
));
break;
case CoverageType.COBERTURA:
await this.xmlExtractCobertura(
coverages,
parsePromises.push(this.xmlExtractCobertura(
fileName,
fileContent
);
));
break;
case CoverageType.LCOV:
this.lcovExtract(coverages, fileName, fileContent);
parsePromises.push(this.lcovExtract(fileName, fileContent));
break;
default:
break;
}
}

const results = await Promise.all(parsePromises);
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

Ok cool, this will have all the promises race each other instead of doing the awaits in series like we had before.

const flattenedSections = results.reduce((acc, val) => acc.concat(val), []);
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

Not 100% sure what this is doing, maybe a small comment here would help?

await this.addSections(coverages, flattenedSections);
Copy link
Copy Markdown
Owner

@ryanluker ryanluker Feb 1, 2026

Choose a reason for hiding this comment

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

Ok this makes sense, it moves the coverage adding to the very end instead of per file 👍🏻.


return coverages;
}

Expand All @@ -80,16 +83,15 @@ export class CoverageParser {
}

private xmlExtractCobertura(
coverages: Map<string, Section>,
coverageFilename: string,
xmlFile: string
) {
return new Promise<void>((resolve) => {
return new Promise<Section[]>((resolve) => {
const checkError = (err: Error) => {
if (err) {
err.message = `filename: ${coverageFilename} ${err.message}`;
this.handleError("cobertura-parse", err);
return resolve();
return resolve([]);
}
};

Expand All @@ -98,8 +100,7 @@ export class CoverageParser {
xmlFile,
async (err, data) => {
checkError(err);
await this.addSections(coverages, data);
return resolve();
return resolve(data);
},
true
);
Expand All @@ -111,24 +112,22 @@ export class CoverageParser {
}

private xmlExtractJacoco(
coverages: Map<string, Section>,
coverageFilename: string,
xmlFile: string
) {
return new Promise<void>((resolve) => {
return new Promise<Section[]>((resolve) => {
const checkError = (err: Error) => {
if (err) {
err.message = `filename: ${coverageFilename} ${err.message}`;
this.handleError("jacoco-parse", err);
return resolve();
return resolve([]);
}
};

try {
parseContentJacoco(xmlFile, async (err, data) => {
checkError(err);
await this.addSections(coverages, data);
return resolve();
return resolve(data);
});
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
Expand All @@ -138,39 +137,37 @@ export class CoverageParser {
}

private async xmlExtractClover(
coverages: Map<string, Section>,
coverageFilename: string,
xmlFile: string
) {
): Promise<Section[]> {
try {
const data = await parseContentClover(xmlFile);
await this.addSections(coverages, data);
return data;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
error.message = `filename: ${coverageFilename} ${error.message}`;
this.handleError("clover-parse", error);
return [];
}
}

private lcovExtract(
coverages: Map<string, Section>,
coverageFilename: string,
lcovFile: string
) {
return new Promise<void>((resolve) => {
return new Promise<Section[]>((resolve) => {
const checkError = (err: Error) => {
if (err) {
err.message = `filename: ${coverageFilename} ${err.message}`;
this.handleError("lcov-parse", err);
return resolve();
return resolve([]);
}
};

try {
source(lcovFile, async (err, data) => {
checkError(err);
await this.addSections(coverages, data);
return resolve();
return resolve(data);
});
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (error: any) {
Expand Down
Loading
Loading