Repository navigation
Refactor name formatting and speedscope parsing - #54
rogeralsing wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the profiler’s name formatting and speedscope JSON parsing into smaller, focused components, while adding characterization tests to lock in expected behavior across invalid inputs and unit/timing scenarios.
Changes:
- Split name formatting into
CompilerGeneratedMethodDisplayFormatterandTypeDisplayNameFormatter, withNameFormatterdelegating to them. - Split speedscope parsing into a thin
SpeedscopeParserfacade plus document reading, unit resolution, aggregation state, and separate processors for evented vs sampled profiles. - Added/expanded tests covering invalid document shapes, missing samples, timing/self-time bounds, unit conversions, unknown frames, and mixed profile labeling.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Asynkron.Profiler.Tests/SpeedscopeParserTests.cs | Adds characterization coverage for invalid shapes, timing/self-time, unit conversion, unknown frames, and mixed profiles. |
| tests/Asynkron.Profiler.Tests/NameFormatterTests.cs | Extends test matrix for compiler-generated lambdas and nested-type display normalization. |
| src/ProfilerCore/Asynkron.Profiler.Core.csproj | Ensures the new formatter helpers are compiled into the core assembly. |
| src/ProfileTool/Rendering/NameFormatter.cs | Delegates compiler-generated method and type formatting to new helper classes. |
| src/ProfileTool/Rendering/CompilerGeneratedMethodDisplayFormatter.cs | New helper for state machine and lambda-style compiler-generated method name normalization. |
| src/ProfileTool/Rendering/TypeDisplayNameFormatter.cs | New helper for stripping namespaces, normalizing generics/arrays, and nested type display. |
| src/ProfileTool/Cpu/SpeedscopeParser.cs | Refactored into a facade calling reader/unit resolver/processors/state. |
| src/ProfileTool/Cpu/SpeedscopeDocumentReader.cs | New reader for validating/reading document shape and frames/weights. |
| src/ProfileTool/Cpu/SpeedscopeProfileUnitResolver.cs | New unit parsing + time scaling logic. |
| src/ProfileTool/Cpu/SpeedscopeAggregationState.cs | New shared aggregation state and result construction. |
| src/ProfileTool/Cpu/SpeedscopeEventedProfileProcessor.cs | New processor for evented profiles (timing + self-time). |
| src/ProfileTool/Cpu/SpeedscopeSampledProfileProcessor.cs | New processor for sampled profiles (weights + call/time accumulation). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| child.Calls += callWeight; | ||
| state.AddFrameTime(frameIdx, timeWeight); | ||
| state.AddFrameCalls(frameIdx, callWeight); | ||
| return child; | ||
| } |
There was a problem hiding this comment.
In sampled processing, child.Calls += callWeight bypasses CallTreeNode.IncrementCalls() clamping and can overflow int (yielding negative/incorrect call counts) for large profiles. Consider clamping when adding callWeight (and similarly for per-frame call aggregation) so call counts never exceed int.MaxValue.
| child.Calls += callWeight; | |
| state.AddFrameTime(frameIdx, timeWeight); | |
| state.AddFrameCalls(frameIdx, callWeight); | |
| return child; | |
| } | |
| var effectiveCallWeight = ClampAndIncrementCalls(child, callWeight); | |
| state.AddFrameTime(frameIdx, timeWeight); | |
| state.AddFrameCalls(frameIdx, effectiveCallWeight); | |
| return child; | |
| } | |
| private static int ClampAndIncrementCalls(CallTreeNode node, int callWeight) | |
| { | |
| if (callWeight <= 0) | |
| { | |
| return 0; | |
| } | |
| var currentCalls = node.Calls; | |
| if (currentCalls >= int.MaxValue) | |
| { | |
| // Already at or above the maximum representable call count. | |
| node.Calls = int.MaxValue; | |
| return 0; | |
| } | |
| var maxIncrement = int.MaxValue - currentCalls; | |
| var effectiveIncrement = callWeight > maxIncrement ? maxIncrement : callWeight; | |
| node.Calls = currentCalls + effectiveIncrement; | |
| return effectiveIncrement; | |
| } |
| public void AddFrameCalls(int frameIdx, int calls) | ||
| { | ||
| _frameCounts.TryGetValue(frameIdx, out var currentCalls); | ||
| _frameCounts[frameIdx] = currentCalls + calls; |
There was a problem hiding this comment.
AddFrameCalls adds into an int dictionary with currentCalls + calls and can overflow for large inputs (producing negative call counts and breaking ordering/labels). Consider clamping to int.MaxValue (or switching the counters to long if higher fidelity is needed) to match CallTreeNode.IncrementCalls() overflow handling.
| _frameCounts[frameIdx] = currentCalls + calls; | |
| long newValue = (long)currentCalls + calls; | |
| if (newValue > int.MaxValue) | |
| { | |
| newValue = int.MaxValue; | |
| } | |
| _frameCounts[frameIdx] = (int)newValue; |
| var timeout = TimeSpan.FromMilliseconds(100); | ||
| var normalized = Regex.Replace( | ||
| name, | ||
| @"\b(?:[A-Za-z_][A-Za-z0-9_]*\.)+(?<type>[A-Za-z_][A-Za-z0-9_]*)", | ||
| "${type}", | ||
| RegexOptions.CultureInvariant | RegexOptions.ExplicitCapture, | ||
| timeout); |
There was a problem hiding this comment.
This formatter uses several Regex.Replace calls with inline patterns on every invocation. Elsewhere in the repo regex patterns are typically implemented via [GeneratedRegex] (or otherwise cached) to avoid repeated regex parsing/cache churn; consider switching these patterns to generated/cached regexes for consistency and performance.
Summary
NameFormatterinto focused helpers for compiler-generated method names and type-display normalization.SpeedscopeParserinto a thin facade plus document reading, unit resolution, aggregation state, and separate evented/sampled processors.Asynkron.Profiler.Core.csprojso the shared formatter helpers are compiled into the core assembly.Testing
dotnet format Asynkron.Profiler.sln --verify-no-changesdotnet build Asynkron.Profiler.sln -warnaserror --disable-build-serversdotnet test Asynkron.Profiler.sln --disable-build-serversquickdup -path src -ext .cs -select 0..20 -min 2 -exclude ".g.,.generated."Notes
roslynator fix Asynkron.Profiler.slnwas attempted as part of the pre-PR flow, but the currentroslynator.dotnet.clirelease is incompatible with this repo's .NET 10 preview SDK and fails while loading the solution. The repo was therefore validated withdotnet formatplus warning-as-error build/test instead.