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

init capacity #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

init capacity #96

wants to merge 1 commit into from

Conversation

Rootjhon
Copy link
Contributor

  1. ToString avoid StringBuilder resize due to capacity
  2. ToDebugString chang append (string) -> append(char)

avoid StringBuilder resize due to capacity
Copy link
Collaborator

@hk1ll3r hk1ll3r left a comment

Choose a reason for hiding this comment

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

I'm not sure if this tradeoff between space and exec time is desired.

If you have the time to compute the string length from UTF32 code points in a helper method like ComputeStringLength(), we can use that here.

@@ -161,7 +161,7 @@ public class FastStringBuilder {
}

public override string ToString() {
StringBuilder sb = new StringBuilder();
StringBuilder sb = new StringBuilder(length * 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we should set the capacity to the eventual length of the string which we could compute by replicating the surrogate pair logic in our code. 2*length is an upper bound and allocates twice as much memory as is needed in most cases.

@@ -170,10 +170,10 @@ public class FastStringBuilder {

public string ToDebugString()
{
StringBuilder sb = new StringBuilder();
StringBuilder sb = new StringBuilder(length * 3);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not worry about performance of debug code.

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.

None yet

2 participants