Repository navigation
Update benchmark.yml - #31
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the benchmark GitHub Actions workflow to avoid scheduling unnecessary OS matrix jobs except when manually dispatched, reducing CI load for push/PR events.
Changes:
- Make
strategy.matrix.osdynamic so push/PR runs only execute onubuntu-latest, whileworkflow_dispatchcan target all OSes. - Simplify the run-gating PowerShell logic by defaulting
$shouldRunto true and only applying input-based gating for manual dispatches.
Comments suppressed due to low confidence (1)
.github/workflows/benchmark.yml:53
$shouldRunnow defaults to$true. If a new/typo OS value ever gets added to the matrix without updating theswitch, it will run unintentionally onworkflow_dispatch. Safer pattern is to default to$falsewithin the dispatch branch (or add adefaultcase) so only explicitly handled OS values are allowed to proceed.
$shouldRun = $true
if ($eventName -eq "workflow_dispatch") {
switch ($os) {
"ubuntu-latest" { $shouldRun = "${{ inputs.run_ubuntu }}" -eq "true"; break }
"windows-latest" { $shouldRun = "${{ inputs.run_windows }}" -eq "true"; break }
"macos-latest" { $shouldRun = "${{ inputs.run_macos }}" -eq "true"; break }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fail-fast: false | ||
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| os: ${{ fromJson(github.event_name == 'workflow_dispatch' && '["ubuntu-latest","windows-latest","macos-latest"]' || '["ubuntu-latest"]') }} |
There was a problem hiding this comment.
For workflow_dispatch, the matrix always schedules all 3 OS jobs even when an input (e.g. run_windows) is false; those jobs still provision a runner just to run the gate step and then skip the rest. Consider building the matrix.os list from the dispatch inputs so disabled OS values are omitted entirely, reducing runner usage and queue time.
| os: ${{ fromJson(github.event_name == 'workflow_dispatch' && '["ubuntu-latest","windows-latest","macos-latest"]' || '["ubuntu-latest"]') }} | |
| os: ${{ fromJson(github.event_name == 'workflow_dispatch' && format('[{0}{1}{2}]', | |
| inputs.run_ubuntu && '"ubuntu-latest"' || '', | |
| inputs.run_windows && (inputs.run_ubuntu && ',"windows-latest"' || '"windows-latest"') || '', | |
| inputs.run_macos && ((inputs.run_ubuntu || inputs.run_windows) && ',"macos-latest"' || '"macos-latest"') || '' | |
| ) || '["ubuntu-latest"]') }} |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Safe-type shortcut misses boxed value type exclusion
- Added !runtimeType.IsValueType check to the fast-path condition in Clone1DimArrayClassInternal to exclude boxed value types, matching the pattern used in CloneClassShallowAndTrack and BuildTypeMetadata.
…ClassInternal The fast-path optimization at line 979 only checked CanReturnSameObject(runtimeType) without excluding value types. When T is object or an interface, array elements can be boxed structs that must be deep-cloned (boxed structs can be mutated). This fix adds the same !runtimeType.IsValueType check used in CloneClassShallowAndTrack and BuildTypeMetadata to ensure boxed value types are properly deep-cloned.
Deep Clone Benchmarks
Current FastCloner vs DeepCloner
FastCloner vs latest
|
| Status | Benchmark | FC Time (Baseline) | FC Time (Current) | Delta Time | FC Alloc (Baseline) | FC Alloc (Current) | Delta Alloc |
|---|---|---|---|---|---|---|---|
| 🟢 | DynamicWithArray | 6,359.98 ns | 5,023.64 ns | -21% faster | 2,840 B | 2,744 B | -3% less |
| ⚪ | DynamicWithDictionary | 1,024.11 ns | 1,074.06 ns | +5% slower | 1,600 B | 1,600 B | ~same |
| ⚪ | DynamicWithNestedObject | 1,344.40 ns | 1,313.28 ns | -2% faster | 1,776 B | 1,776 B | ~same |
| ⚪ | FileSpec | 297.28 ns | 298.40 ns | ~same | 416 B | 416 B | ~same |
| ⚪ | LargeEventDocument_10MB | 37,918.57 ns | 38,058.96 ns | ~same | 49,824 B | 49,824 B | ~same |
| 🔴 | LargeLogBatch_10MB | 3,118,356.96 ns | 3,671,896.53 ns | +18% slower | 2,649,094 B | 2,649,104 B | ~same |
| ⚪ | MediumNestedObject | 1,121.19 ns | 1,110.34 ns | ~same | 1,616 B | 1,616 B | ~same |
| ⚪ | ObjectDictionary_50 | 178,335.37 ns | 183,984.76 ns | +3% slower | 218,768 B | 218,768 B | ~same |
| ⚪ | ObjectList_100 | 107,796.83 ns | 107,334.11 ns | ~same | 149,816 B | 149,816 B | ~same |
| ⚪ | SmallObject | 73.12 ns | 74.28 ns | ~same | 48 B | 48 B | ~same |
| 🟢 | SmallObjectWithCollections | 386.55 ns | 361.67 ns | -6% faster | 576 B | 576 B | ~same |
| 🟢 | StringArray_1000 | 623.53 ns | 518.35 ns | -17% faster | 8,024 B | 8,024 B | ~same |
Regressions
LargeLogBatch_10MB: time +18% slower, alloc ~same
Improvements
DynamicWithArray: time -21% faster, alloc -3% lessSmallObjectWithCollections: time -6% faster, alloc ~sameStringArray_1000: time -17% faster, alloc ~same
Mixed changes
- none
…into chore-ci-bench
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private struct TypeCloneDispatchCache2 | ||
| { | ||
| private Type? typeA; | ||
| private FastClonerCache.TypeCloneMetadata? metadataA; | ||
| private Func<object, FastCloneState, object>? recursiveA; | ||
| private Func<object, FastCloneState, object>? worklistA; | ||
| private Type? typeB; | ||
| private FastClonerCache.TypeCloneMetadata? metadataB; | ||
| private Func<object, FastCloneState, object>? recursiveB; | ||
| private Func<object, FastCloneState, object>? worklistB; | ||
| private Type? typeC; | ||
| private FastClonerCache.TypeCloneMetadata? metadataC; | ||
| private Func<object, FastCloneState, object>? recursiveC; | ||
| private Func<object, FastCloneState, object>? worklistC; | ||
| private Type? typeD; | ||
| private FastClonerCache.TypeCloneMetadata? metadataD; | ||
| private Func<object, FastCloneState, object>? recursiveD; | ||
| private Func<object, FastCloneState, object>? worklistD; |
There was a problem hiding this comment.
TypeCloneDispatchCache2 now holds 4 cached entries (A–D). The "2" suffix is misleading and makes the cache behavior harder to reason about when revisiting this code. Consider renaming the struct to reflect its actual capacity (e.g., TypeCloneDispatchCache4) or removing the numeric suffix if it no longer conveys meaning.
Note
Medium Risk
Touches core cloning hot paths (type-dispatch caching and class-array cloning fast paths), which could affect clone correctness/perf for polymorphic arrays and override scenarios. CI/reporting changes are low risk but could alter baseline selection and diff presentation.
Overview
Updates the benchmark GitHub Actions workflow to run only
ubuntu-latestby default (while keeping optional multi-OS runs forworkflow_dispatch) and switches baseline retrieval togh run downloadusing the originating run id.Improves benchmark diff output formatting by moving
Statusto the first column and rendering statuses as symbols (🔴/🟢/🟡/🆕/⚪), and slightly simplifies CSV time parsing by consolidating microsecond unit handling.Optimizes cloning internals by expanding
TypeCloneDispatchCache2from a 2-entry to a 4-entry promotion cache and adding a fast path inClone1DimArrayClassInternalto directly copy safe/primitive/enum elements when no optional type behavior overrides are active.Written by Cursor Bugbot for commit f9a0102. This will update automatically on new commits. Configure here.