Skip to content

Reuse a TimeCode instance in TimeSpanToDisplayFullConverter - #13221

Merged
niksedk merged 3 commits into
SubtitleEdit:mainfrom
ivandrofly:perf/timespan-display-converter
Aug 5, 2026
Merged

Reuse a TimeCode instance in TimeSpanToDisplayFullConverter#13221
niksedk merged 3 commits into
SubtitleEdit:mainfrom
ivandrofly:perf/timespan-display-converter

Conversation

@ivandrofly

@ivandrofly ivandrofly commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Reuse a private TimeCode instance in TimeSpanToDisplayFullConverter.Convert instead of allocating a new one on every call
  • Return cached string constants for the zero-time fallbacks
  • Parse ConvertBack input with allocation-free Span.SplitAny instead of string.Split, collapsing the duplicated frame-mode/time-mode branches into one
  • The converter runs for every visible time cell binding, so this trims steady-state GC pressure in the main grid

Benchmark: Convert

BenchmarkDotNet (ShortRun, .NET 10.0.10, X64 RyuJIT AVX2, i7-13700), mirroring the converter's old vs new formatting path:

Method Mean Allocated
Old: new TimeCode(ts).ToString() 33.76 ns 72 B
New: reused instance ToString() 31.26 ns 48 B
Old: new TimeCode(ts).ToHHMMSSFF() 35.13 ns 72 B
New: reused instance ToHHMMSSFF() 33.74 ns 48 B

The change removes the 24 B TimeCode allocation per call (the remaining 48 B is the result string itself); the time delta is minor. Note the converter is only used from Avalonia bindings on the UI thread, so the shared mutable instance is safe there.

Benchmark: ConvertBack

Same setup, old string.Split parsing vs the new span-based SplitAny:

Input Old (string.Split) New (SplitAny span) Ratio Alloc old → new
00:01:23,456 (ms mode) 53.4 ns 30.1 ns 0.56 208 B → 24 B
00:01:23,456 (frame mode) 54.2 ns 30.5 ns 0.56 208 B → 24 B
01:02:03.12 52.2 ns 29.2 ns 0.56 208 B → 24 B
not a time (invalid) 14.8 ns 8.9 ns 0.60 56 B → 24 B

~1.8× faster; the 184 B saved per call is the string[] plus the four substring allocations from Split. The remaining 24 B is the TimeSpan box forced by IValueConverter returning object, paid identically in both versions.

Test plan

  • Open a subtitle file and verify start/end/duration times display unchanged in the grid
  • Toggle frame mode (hh:mm:ss.ff) and verify times render correctly
  • Set a video offset (CurrentVideoOffsetInMs ≠ 0) and verify displayed times include the offset in both time and frame mode
  • Verify empty/unset time cells show 00:00:00,000 (time mode) / 00:00:00.00 (frame mode)
  • Edit a time cell and verify the typed value round-trips correctly in both time and frame mode, and that invalid input falls back to 00:00:00,000

🤖 Generated with Claude Code

Avoid allocating a new TimeCode on every Convert call by reusing a
private instance, and return cached constants for the zero-time
fallbacks.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@ivandrofly

Copy link
Copy Markdown
Member Author
image

…onvertBack

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Optimizes TimeSpanToDisplayFullConverter to reduce steady-state allocations/GC pressure during UI grid rendering by reusing formatting state and switching parsing to span-based tokenization.

Changes:

  • Reuse a single TimeCode instance during Convert to avoid per-call TimeCode allocations.
  • Cache zero-time fallback strings for frame/time mode.
  • Refactor ConvertBack parsing to use Span.SplitAny and unify frame/time parsing paths.

Comment thread src/ui/Logic/ValueConverters/TimeSpanToDisplayFullConverter.cs Outdated
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@niksedk
niksedk merged commit 6656a17 into SubtitleEdit:main Aug 5, 2026
1 check passed
@niksedk niksedk mentioned this pull request Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants