Skip to content

fix Duplicate DrawString#604

Open
7aGiven wants to merge 4 commits into
FNA-XNA:masterfrom
7aGiven:duplicateDrawString
Open

fix Duplicate DrawString#604
7aGiven wants to merge 4 commits into
FNA-XNA:masterfrom
7aGiven:duplicateDrawString

Conversation

@7aGiven

@7aGiven 7aGiven commented Jul 18, 2026

Copy link
Copy Markdown
Contributor
  • I confirm that I am the author of this code and release it to the FNA project under the Ms-PL license. This contribution does not contain code from other sources, including code generated by a Large Language Model ("AI").

Try to unify string class and StringBuilder class.

Core code:

		internal interface IReadOnlyCharList
		{
			int Length { get; }
			char this[int index] { get; }
		}
		internal struct StringView : IReadOnlyCharList
		{
			private readonly string view;
			internal StringView(string view)
			{
				this.view = view;
			}
			public int Length
			{
				get
				{
					return view.Length;
				}
			}
			public char this[int index]
			{
				get
				{
					return view[index];
				}
			}
		}
		internal struct StringBuilderView : IReadOnlyCharList
		{
			private readonly StringBuilder view;
			internal StringBuilderView(StringBuilder view)
			{
				this.view = view;
			}
			public int Length
			{
				get
				{
					return view.Length;
				}
			}
			public char this[int index]
			{
				get
				{
					return view[index];
				}
			}
		}

@flibitijibibo flibitijibibo left a comment

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.

Mostly focused on style for this pass but this seems okay; since the reader is a struct I don't anticipate additional memory pressure, but I'll ask others to see if this has any measurable effect on performance.

Comment thread src/Graphics/SpriteFont.cs Outdated
Comment thread src/Graphics/SpriteFont.cs
Co-authored-by: Ethan Lee <flibitijibibo@gmail.com>
int Length { get; }
char this[int index] { get; }
}
internal struct StringView : IReadOnlyCharList

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.

Making this a struct is pointless since you're going to box it anyway, it would be better as a sealed class to read how it's going to behave

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I follow your suggestion that use generic method instead of interface argument. So no boxing now.

Comment thread src/Graphics/SpriteFont.cs Outdated
#region Internal MeasureString Method

public Vector2 MeasureString(string text)
internal Vector2 MeasureString(ref IReadOnlyCharList text)

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.

ref just adds overhead here because the argument is of a boxed type (an interface). If you had MeasureString<T>(ref T text) where T : IReadOnlyCharList then ref would make sense and you could avoid boxing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I have not get the knowledge before. I know it now from https://giannisakritidis.com/blog/Avoid-Struct-Boxing/

Comment thread src/Graphics/SpriteFont.cs Outdated
Comment thread src/Graphics/SpriteBatch.cs Outdated
Comment on lines +715 to +716
SpriteFont.IReadOnlyCharList view = new SpriteFont.StringBuilderView(text);
DrawString(spriteFont, ref view, position, color, rotation, origin, scale, effects, layerDepth);

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.

As mentioned in another comment, by construction this boxes the StringBuilderView to store it in a reference-typed (IReadOnlyCharList) local, then passes a reference to the reference, which is unnecessary. Either make DrawString generic so you can pass structs into it by-ref or drop the ref here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you, I follow your suggestion that use generic method instead of interface argument. So no boxing now.

@kg

kg commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Thanks for doing this refactoring. Have you tested it using VS's memory profiler to see whether it allocates? It's possible the modern .NET JIT is smart enough to optimize out some of the boxing here but I would expect it to box each adapter instance.

@7aGiven

7aGiven commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for doing this refactoring. Have you tested it using VS's memory profiler to see whether it allocates? It's possible the modern .NET JIT is smart enough to optimize out some of the boxing here but I would expect it to box each adapter instance.

I find that there is box instruction in dotnet IL. So I think it must be fix regardless of whether the JIT can optimize it.

@kg

kg commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

I can't easily test this locally because I don't have any spritefont-based games lying around but the new code looks good.

@flibitijibibo flibitijibibo self-assigned this Jul 20, 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