Skip to content

Optimize StatusLine with unsafe#14

Closed
stefannikolei wants to merge 1 commit intoSixLabors:mainfrom
stefannikolei:sn/perf/statusline
Closed

Optimize StatusLine with unsafe#14
stefannikolei wants to merge 1 commit intoSixLabors:mainfrom
stefannikolei:sn/perf/statusline

Conversation

@stefannikolei
Copy link
Copy Markdown
Contributor

Just trying things out how AI can help in this cases. I think Claude Sonnet 4 did a pretty reasonable job.

Copilot AI review requested due to automatic review settings July 6, 2025 13:49
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors StatusLine to use a raw SweepEvent[] buffer with manual resizing and unsafe access for faster inserts/removals.

  • Replace List<SweepEvent> with an internal array plus a count field
  • Use Unsafe.Add and MemoryMarshal.GetArrayDataReference for element access
  • Implement custom binary-search insertion, shifting via Array.Copy, and position updates

{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => this.sortedEvents[index];
get => Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.events), index);
Copy link

Copilot AI Jul 6, 2025

Choose a reason for hiding this comment

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

The indexer bypasses bounds checks, which can lead to out-of-range reads/writes. Add an explicit check (e.g., if (index < 0 || index >= this.count) throw new ArgumentOutOfRangeException(nameof(index));) before the unsafe access.

Suggested change
get => Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.events), index);
get
{
if (index < 0 || index >= this.count)
{
throw new ArgumentOutOfRangeException(nameof(index));
}
return Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.events), index);
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The bot has a point. We've lost the safety of a bounds check here.

/// </exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void RemoveAt(int index)
{
Copy link

Copilot AI Jul 6, 2025

Choose a reason for hiding this comment

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

RemoveAt does not validate that index is within [0, Count). Invalid indices will cause undefined behavior. Consider adding a bounds check and throwing ArgumentOutOfRangeException to match the original List<T> semantics.

Suggested change
{
{
// Validate index
if (index < 0 || index >= this.count)
{
throw new ArgumentOutOfRangeException(nameof(index), "Index must be within the range [0, Count).");
}

Copilot uses AI. Check for mistakes.
Comment on lines +199 to +200
// TODO: Is this the best growth strategy?
int newCapacity = this.events.Length + 16;
Copy link

Copilot AI Jul 6, 2025

Choose a reason for hiding this comment

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

Growing the array by a fixed amount (+16) leads to O(n²) behavior when many inserts occur. Switch to exponential growth (e.g., newCapacity = this.events.Length * 2) for amortized O(1) resizes.

Suggested change
// TODO: Is this the best growth strategy?
int newCapacity = this.events.Length + 16;
// TODO: Switch to exponential growth for better performance.
int newCapacity = this.events.Length == 0 ? 16 : this.events.Length * 2;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

I was going to do a thorough review here, but Claude has made far too unnecessary changes. I don't think it's safe to rely on AI for these kinds of changes, the result is often very poor.

We should also be providing unit tests and benchmarks to proof performance improvements.

{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
get => this.sortedEvents[index];
get => Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(this.events), index);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The bot has a point. We've lost the safety of a bounds check here.

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