Skip to content
23 changes: 16 additions & 7 deletions src/addBenchmarkEntry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { Benchmark } from './extract';
import * as core from '@actions/core';
import { BenchmarkSuites } from './write';
import { normalizeBenchmark } from './normalizeBenchmark';
import { GitGraphAnalyzer } from './gitGraph';

export function addBenchmarkEntry(
benchName: string,
Expand All @@ -11,24 +12,32 @@ export function addBenchmarkEntry(
): { prevBench: Benchmark | null; normalizedCurrentBench: Benchmark } {
let prevBench: Benchmark | null = null;
let normalizedCurrentBench: Benchmark = benchEntry;
const gitAnalyzer = new GitGraphAnalyzer();

// Add benchmark result
if (entries[benchName] === undefined) {
entries[benchName] = [benchEntry];
core.debug(`No suite was found for benchmark '${benchName}' in existing data. Created`);
} else {
const suites = entries[benchName];
// Get the last suite which has different commit ID for alert comment
for (const e of [...suites].reverse()) {
if (e.commit.id !== benchEntry.commit.id) {
prevBench = e;
break;
}

// Find previous benchmark using git ancestry
core.debug(`Finding previous benchmark for commit: ${benchEntry.commit.id}`);

prevBench = gitAnalyzer.findPreviousBenchmark(suites, benchEntry.commit.id);
Copy link
Member

Choose a reason for hiding this comment

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

This is an interesting approach. I think it is a good one, but I'm not sure that we should make it the new default just yet. I'd prefer to add a config option to the action so that it's an opt-in feature unless we can comprehensively test it so that we are sure that it's backwards compatible.


if (prevBench) {
core.debug(`Found previous benchmark: ${prevBench.commit.id}`);
} else {
core.debug('No previous benchmark found');
}

normalizedCurrentBench = normalizeBenchmark(prevBench, benchEntry);

suites.push(normalizedCurrentBench);
// Insert at the correct position based on git ancestry
const insertionIndex = gitAnalyzer.findInsertionIndex(suites, benchEntry.commit.id);
core.debug(`Inserting benchmark at index ${insertionIndex} (of ${suites.length} existing entries)`);
suites.splice(insertionIndex, 0, normalizedCurrentBench);

if (maxItems !== null && suites.length > maxItems) {
suites.splice(0, suites.length - maxItems);
Expand Down
1 change: 1 addition & 0 deletions src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as os from 'os';
import * as path from 'path';

export type ToolType = typeof VALID_TOOLS[number];

export interface Config {
name: string;
tool: ToolType;
Expand Down
13 changes: 8 additions & 5 deletions src/default_index_html.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,11 +161,14 @@ export const DEFAULT_INDEX_HTML = String.raw`<!DOCTYPE html>
a.click();
};
// Prepare data points for charts
return Object.keys(data.entries).map(name => ({
name,
dataSet: collectBenchesPerTestCase(data.entries[name]),
}));
// Prepare data points for charts (uses server-side ordering)
return Object.keys(data.entries).map(name => {
const entries = data.entries[name];
return {
name,
dataSet: collectBenchesPerTestCase(entries),
};
});
Comment on lines +164 to +171
Copy link
Member

Choose a reason for hiding this comment

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

This change seems to be just stylistic, right? Let's revert as it's not relevant to this change

}
function renderAllChars(dataSets) {
Expand Down
154 changes: 154 additions & 0 deletions src/gitGraph.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,154 @@
import { spawnSync } from 'child_process';
import * as core from '@actions/core';
import { Benchmark } from './extract';

export class GitGraphAnalyzer {
private readonly gitCliAvailable: boolean;

constructor() {
try {
spawnSync('git', ['--version'], { stdio: 'ignore' });
this.gitCliAvailable = true;
} catch (e) {
this.gitCliAvailable = false;
core.debug('Git CLI not available during initialization');
}
}

/**
* Check if git CLI is available
*/
isGitAvailable(): boolean {
return this.gitCliAvailable;
}

/**
* Validate that a ref matches expected git reference patterns.
* Accepts SHA hashes, branch names, and tag names.
*/
private isValidRef(ref: string): boolean {
if (!ref || ref.length === 0) {
return false;
}
// Allow: hex SHA (short or full), branch/tag names (alphanumeric, dots, underscores, hyphens, slashes)
const validRefPattern = /^[a-zA-Z0-9._\-/]+$/;
return validRefPattern.test(ref);
}

/**
* Get git ancestry using topological order
*/
getAncestry(ref: string): string[] {
if (!this.gitCliAvailable) {
core.warning('Git CLI not available, cannot determine ancestry');
return [];
}

if (!this.isValidRef(ref)) {
core.warning(`Invalid git ref format: ${ref}`);
return [];
}

try {
const result = spawnSync('git', ['log', '--oneline', '--topo-order', ref], {
encoding: 'utf8',
cwd: process.env.GITHUB_WORKSPACE ?? process.cwd(),
});

if (result.error) {
throw result.error;
}

return result.stdout
.split('\n')
.filter((line) => line.trim())
.map((line) => line.split(' ')[0]); // Extract SHA from "sha message"
Comment on lines +62 to +65
Copy link
Member

Choose a reason for hiding this comment

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

worth noting that for large repos, this might be huge. Something to keep an eye on. That's another reason why I'd like to have this feature opt-in until we verify it's not causing issues for current users

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could add a reasonable limit to the git command so that we don't query for all the commits until the beginning of the repo

} catch (error) {
core.warning(`Failed to get ancestry for ref ${ref}: ${error}`);
return [];
}
}

/**
* Find previous benchmark commit based on git ancestry.
* Falls back to execution time ordering if git ancestry is not available.
*/
findPreviousBenchmark(suites: Benchmark[], currentSha: string): Benchmark | null {
const ancestry = this.getAncestry(currentSha);

if (ancestry.length === 0) {
core.warning(`No ancestry found for commit ${currentSha}, falling back to execution time ordering`);
return this.findPreviousByExecutionTime(suites, currentSha);
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest not intermingling the git graph logic with the previous logic, and instead return null here and move the fallback handling to the caller

}

// Find position of current commit in ancestry
const currentIndex = ancestry.indexOf(currentSha);
if (currentIndex === -1) {
core.warning(`Current commit ${currentSha} not found in ancestry, falling back to execution time ordering`);
return this.findPreviousByExecutionTime(suites, currentSha);
Copy link
Member

Choose a reason for hiding this comment

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

}

// Look for next commit in ancestry that exists in benchmarks
for (let i = currentIndex + 1; i < ancestry.length; i++) {
const previousSha = ancestry[i];
const previousBenchmark = suites.find((suite) => suite.commit.id === previousSha);

if (previousBenchmark) {
core.debug(`Found previous benchmark: ${previousSha} based on git ancestry`);
return previousBenchmark;
}
}

// Fallback: no previous commit found in ancestry
core.debug('No previous benchmark found in git ancestry');
return null;
}

/**
* Find the insertion index for a new benchmark entry based on git ancestry.
* Inserts after the most recent ancestor in the existing suites.
*/
findInsertionIndex(suites: Benchmark[], newCommitSha: string): number {
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought. findInsertionIndex and findPreviousBenchmark feel like they are conceptually very similar. Couldn't we refactor so that we can reuse more logic?

something like

function findPreviousBenchmarkIndex(/*params*/) {
 // finding the previous bench idx
}

function findPreviousBenchmark(...) {
// simplified implementation
  return benches[findPreviousBenchmarkIndex()]
}

function findInsertionIndex() {
  // simplified
  return findPreviousBenchmark() + 1
}

if (!this.gitCliAvailable || suites.length === 0) {
return suites.length;
}

const ancestry = this.getAncestry(newCommitSha);
if (ancestry.length === 0) {
core.debug('No ancestry found, appending to end');
return suites.length;
Copy link
Member

Choose a reason for hiding this comment

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

similar to the logic used to find the previous benchmark. It think it's cleaner to separate the git graph logic from the "inserting at the end" as a fallback. If we return null or -1 here we could move the fallback logic to the addBenchmarkEntry

}

// Create a set of ancestor SHAs for quick lookup (excluding the commit itself)
// Skip first element only if it matches the commit (it should)
const startIndex = ancestry[0] === newCommitSha ? 1 : 0;
const ancestorSet = new Set(ancestry.slice(startIndex));

// Find the most recent ancestor in the existing suites
// Iterate through suites from end to beginning to find the most recent one
for (let i = suites.length - 1; i >= 0; i--) {
const suite = suites[i];
if (ancestorSet.has(suite.commit.id)) {
core.debug(`Found ancestor ${suite.commit.id} at index ${i}, inserting after it`);
return i + 1; // Insert after this ancestor
}
}

// No ancestor found in existing suites - this commit is likely from a different branch
// or is very old. Append to end as fallback.
core.debug('No ancestor found in existing suites, appending to end');
return suites.length;
}

/**
* Fallback method: find previous by execution time (original logic)
*/
private findPreviousByExecutionTime(suites: Benchmark[], currentSha: string): Benchmark | null {
for (const suite of [...suites].reverse()) {
if (suite.commit.id !== currentSha) {
return suite;
}
}
return null;
}
}
2 changes: 1 addition & 1 deletion src/write.ts
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ async function writeBenchmarkToExternalJson(
jsonFilePath: string,
config: Config,
): Promise<{ prevBench: Benchmark | null; normalizedCurrentBench: Benchmark }> {
const { name, maxItemsInChart, saveDataFile } = config;
const { name, saveDataFile, maxItemsInChart } = config;
const data = await loadDataJson(jsonFilePath);
const { prevBench, normalizedCurrentBench } = addBenchmarkToDataJson(name, bench, data, maxItemsInChart);

Expand Down
Loading