Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate range check in VSB.Append(char) #82264

Merged
merged 1 commit into from
Feb 17, 2023
Merged

Remove duplicate range check in VSB.Append(char) #82264

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/libraries/Common/src/System/Text/ValueStringBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,10 @@ public void Insert(int index, string? s)
public void Append(char c)
{
int pos = _pos;
if ((uint)pos < (uint)_chars.Length)
Span<char> chars = _chars;
if ((uint)pos < (uint)chars.Length)
{
_chars[pos] = c;
chars[pos] = c;
Comment on lines +173 to +176
Copy link
Contributor

@dakersnar dakersnar Feb 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Both methods are indexing on a Span<char>, correct? Why does this new method avoid a range check?

Copy link
Member Author

Choose a reason for hiding this comment

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

If both the index and span are locals, and you index into those same locals, the JIT avoids emitting an extra range check and helper for the throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I might have misunderstood. My original assumption was that the if ((uint)pos < (uint)chars.Length) allows the JIT to skip the range check in chars[pos] = c;, is that not the case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that is the case after this change.
But that optimization only works on locals. Before, we were reading the field twice (once in the length check and once in the assignment), and by the time we got to the assignment, the JIT couldn't tell that it already checked the length, so it emitted another one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, that makes sense, thanks for the explanation.

Out of curiosity, do you know what prevents the JIT from optimizing the original case? Is it the potential for the global _chars to be accessed by another thread after the if statement? Or is optimizing globals just out of scope for the JIT for some simpler reason?

Copy link
Member Author

@MihaZupan MihaZupan Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In a general case where you have a field on a struct/class, a different thread could swap out the value from under you between the two reads, so you can't make the assumption that avoiding the second length check is safe.

In this case, this is a field on a ref struct, so it may be a valid assumption? I'll leave that up to smarter people like @EgorBo though.

Copy link
Member

Choose a reason for hiding this comment

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

That's #72004

_pos = pos + 1;
}
else
Expand Down