-
Notifications
You must be signed in to change notification settings - Fork 178
Add git-graph aware sorting for benchmark data and comparisons #329
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
239cd5d
58cb4b5
39095e0
9f0ddca
daa027d
1d2ae9f
ca7156c
941cedb
89af8c3
15b0628
e936a38
e990f9b
549f163
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
|
||
| 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
|
|
||
| // 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a thought. something like |
||
| 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; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
|
|
||
| // 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; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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.