Skip to content

Conversation

@Achilles1515
Copy link
Contributor

This StringBuilder object is being created because the string named "value" needs escaping. It is unknown at this point exactly how many characters need escaping, but AT MINIMUM there is one. This means the new escaped string length will be larger than original string length by at least one character, and the StringBuilder constructor capacity parameter should reflect this logic. Without this change, the StringBuilder object is GUARANTEED to allocate and chain together a new StringBuilder object under the hood as it increases it's capacity to handle the larger escaped string.

This StringBuilder object is being created because the string named "value" needs escaping. It is unknown at this point exactly how many characters need escaping, but AT MINIMUM there is one. This means the new escaped string length will be larger than original string length by at least one character, and the StringBuilder instantiation constructor capacity parameter should reflect this logic. Without this change, the StringBuilder object is GUARANTEED to allocate and chain together a new StringBuilder object under the hood as it increases it's capacity to handle the larger escaped string.
@jyemin
Copy link
Contributor

jyemin commented Nov 27, 2018

Hi @Achilles1515 as per our contributing guide, please open an issue at jira.mongodb.org in the CSHARP project and link it to the pull request.

Thanks,
Jeff

@Achilles1515 Achilles1515 changed the title Increase StringBuilder capacity CSHARP-2442: Increase StringBuilder capacity Nov 27, 2018
@Achilles1515
Copy link
Contributor Author

Hi @Achilles1515 as per our contributing guide, please open an issue at jira.mongodb.org in the CSHARP project and link it to the pull request.

Thanks,
Jeff

Done. Sorry about that.

Copy link
Contributor

@dnickless dnickless left a comment

Choose a reason for hiding this comment

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

How about something like this even:

            int replacements = value.Count(c => NeedsEscaping(c));
            if (replacements == 0)
            {
                return value;
            }

            var sb = new StringBuilder(value.Length + replacements);

Also, NeedsEscaping should be made static as we currently have a lambda closure over "this" here which allocates no matter if we end up escaping things or not.
image
image

Thinking about it a bit more, I would go as far as suggesting to drop the whole temporary string that gets built using the StringBuilder and have EscapedString write to the _textWriter directly instead. Looking at the current usages of EscapedString this appears justifiable.

@BorisDog BorisDog requested review from BorisDog and rstam and removed request for BorisDog February 9, 2024 17:42
@rstam
Copy link
Contributor

rstam commented Mar 13, 2024

This PR was included as part of PR #1290

Thank you for submitting it.

@rstam rstam closed this Mar 13, 2024
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.

4 participants