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.

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

[API Proposal]: Stringbuilder.Replace(ROS oldValue, ROS newValue, int index, int count) #77837

Closed
CodingMadness opened this issue Nov 3, 2022 · 10 comments · Fixed by #93938
Closed
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@CodingMadness
Copy link

CodingMadness commented Nov 3, 2022

Background and motivation

I am operating a lot with spans and slicees and I would super like to not get forced to write-out the span to a string by "builder.ToString()", if possible.

It would save tremendous allocations cause when working in game dev /graphics related stuff , the amount of calls which happen per second make up a significant GC-pressure, so would be nice to avoid that.

API Proposal

  public StringBuilder Replace(ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue, int startIndex, int count)
  public StringBuilder Replace(ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue)

API Usage

string input = "Hello world";

ROS charSpan = bigString.Slice(0, 5);
ROS replacer = stackalloc char[] {'d', 'e', 'a', 'r'};
StringBuilder builder = new(input);

builder.Replace(charSpan, replacer);

var output = builder.ToString(); // --> dear world

Alternative Designs

Due to backward compatibility, it would be only an option to add a new API.

Risks

AFAIK none.

@CodingMadness CodingMadness added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 3, 2022
@ghost
Copy link

ghost commented Nov 3, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

I am operating a lot with spans and slicees and I would super like to not get forced to write-out the span to a string by "builder.ToString()", if possible.

It would save tremendous allocations cause when working in game dev /graphics related stuff , the amount of calls which happen per second make up a significant GC-pressure, so would be nice to avoid that.

API Proposal

  public StringBuilder Replace(ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue, int startIndex, int count)
  public StringBuilder Replace(ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue)

API Usage

string input = "Hello world";

ROS charSpan = bigString.Slice(0, 5);
ROS replacer = stackalloc char[] {'d', 'e', 'a', 'r'};
StringBuilder builder = new(input);

builder.Replace(charSpan, replacer);

Alternative Designs

Due to backward compatibility, it would be only an option to add a new API.

Risks

AFAIK none.

Author: Shpendicus
Assignees: -
Labels:

api-suggestion, area-System.Runtime

Milestone: -

@CodingMadness
Copy link
Author

May I know if this API proposal could be something for .net 8 release?

thx in advance and coolio work you have put in this .NET thing!

@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Oct 3, 2023
@stephentoub
Copy link
Member

These won't make .NET 8, which is all but done at this point.

But the proposal looks reasonable for .NET 9. I've marked it as ready for review. Thanks.

@CodingMadness
Copy link
Author

CodingMadness commented Oct 5, 2023

These won't make .NET 8, which is all but done at this point.

But the proposal looks reasonable for .NET 9. I've marked it as ready for review. Thanks.

Any chance btw to adapt this proposal also for the "AppendJoin()" and "AppendFormat()" methods which still take only string, considering that StringBuilder is used most of the time in high allocation-pressured code , I assume this could be a reasonable adaption aswell.

@stephentoub
Copy link
Member

stephentoub commented Oct 5, 2023

These won't make .NET 8, which is all but done at this point.
But the proposal looks reasonable for .NET 9. I've marked it as ready for review. Thanks.

Any chance btw to adapt this proposal also for the "AppendJoin()" and "AppendFormat()" methods which still take only string, considering that StringBuilder is used most of the time in high allocation-pressured code , I assume this could be a reasonable adaption aswell.

Please open a separate issue with supporting details, real examples of where it'd be helpful, etc. Thanks.

@CodingMadness
Copy link
Author

These won't make .NET 8, which is all but done at this point.
But the proposal looks reasonable for .NET 9. I've marked it as ready for review. Thanks.

Any chance btw to adapt this proposal also for the "AppendJoin()" and "AppendFormat()" methods which still take only string, considering that StringBuilder is used most of the time in high allocation-pressured code , I assume this could be a reasonable adaption aswell.

Please open a separate issue with supporting details, real examples of where it'd be helpful, etc. Thanks.

Not gonna lie I have already had your answer in mind when i was typing, so thats on me ^^.

@TheMaximum
Copy link
Contributor

TheMaximum commented Oct 16, 2023

Just to see how easy it would be to implement something like this, I've already attempted an implementation (see commit below). Was able to change the current implementation with string as arguments to one that takes ReadOnlySpan<char>. The string-methods can then call the ROS one with .AsSpan(). Tests still work, as do the added ones for the ROS variants. Was looking to see if I can somehow benchmark the implementations, but I can't figure out how to run these against the self-built runtime.

Not sure if it's the way you want to go with it, but was educational for me either way.


Provided that this gets through the review meeting, I'd be interested to take a crack at this issue.

Apologies for the original response below, I misunderstood what you're trying to do (a Replace method on StringBuilder, not a Replace method for a ReadOnlySpan<char> in general).


I noticed that a method (ReplaceCore, see below) already exists that accepts ReadOnlySpan<char> as arguments.
It currently returns a string?, but uses a ValueStringBuilder, which could also return the value as ReadOnlySpan<char>.
The ValueStringBuilder doesn't seem to convert to StringBuilder, but maybe a ReadOnlySpan<char> might work as return type as well? In that case ReplaceCore could return the ValueStringBuilder and the calling methods can then decide to return it with .ToString() or .AsSpan().

Don't know if this is the path to take, and other pointers in the right direction are welcome if you're happy for me to have a look at it.

private static string? ReplaceCore(ReadOnlySpan<char> searchSpace, ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue, CompareInfo compareInfo, CompareOptions options)
{
Debug.Assert(!oldValue.IsEmpty);
Debug.Assert(compareInfo != null);
var result = new ValueStringBuilder(stackalloc char[StackallocCharBufferSizeLimit]);

TheMaximum added a commit to TheMaximum/dotnet-runtime that referenced this issue Oct 16, 2023
Converted the StringBuilder.Replace(string, string, int, int) method
and underlaying methods to take ReadOnlySpan<char> as arguments
instead. The Replace methods with string arguments create spans to use
the same new method.

Fix dotnet#77837
@bartonjs
Copy link
Member

bartonjs commented Oct 24, 2023

Video

Looks good as proposed.

namespace System.Text;

partial class StringBuilder
{
   public StringBuilder Replace(ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue, int startIndex, int count);
   public StringBuilder Replace(ReadOnlySpan<char> oldValue, ReadOnlySpan<char> newValue);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Oct 24, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 24, 2023
adamsitnik added a commit that referenced this issue Nov 27, 2023
Fixes #77837

Co-authored-by: Max Klaversma <max.klaversma@vandoren.nl>
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 27, 2023
@CodingMadness
Copy link
Author

will this be ready for .NET 9 or maybe earlier?

@adamsitnik
Copy link
Member

will this be ready for .NET 9 or maybe earlier?

It will be part of .NET 9. We don't backport new features to versions that were already shipped (.NET 8 RTM was shipped 2 weeks ago)

@adamsitnik adamsitnik added this to the 9.0.0 milestone Nov 28, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants