Skip to content

Conversation

@MihaZupan
Copy link
Member

This PR extracts a portion of changes from #34864 to help simplify the review process. As such, #34864 depends on this change.

Rewrite the UnicodeEquivalent helper method into TryGetUnicodeEquivalent, changing:

  • Return bool instead of throwing/catching
  • Work on a string instead of pinning/passing pointers only to allocate a string anyway
  • Instead of building up a string by concatenating each label and allocating a ton of garbage, use a ValueStringBuilder
  • Avoid computing out params that are always discarded
  • Simplify the loop (avoid using state-machine-like flags in a single loop for no reason)

Aside from performance characteristics, Uri behavior should remain completely unchanged.

@MihaZupan MihaZupan added this to the 6.0.0 milestone Jan 8, 2021
@MihaZupan MihaZupan requested review from a team and alnikola January 8, 2021 15:16
@ghost
Copy link

ghost commented Jan 8, 2021

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

Issue Details

This PR extracts a portion of changes from #34864 to help simplify the review process. As such, #34864 depends on this change.

Rewrite the UnicodeEquivalent helper method into TryGetUnicodeEquivalent, changing:

  • Return bool instead of throwing/catching
  • Work on a string instead of pinning/passing pointers only to allocate a string anyway
  • Instead of building up a string by concatenating each label and allocating a ton of garbage, use a ValueStringBuilder
  • Avoid computing out params that are always discarded
  • Simplify the loop (avoid using state-machine-like flags in a single loop for no reason)

Aside from performance characteristics, Uri behavior should remain completely unchanged.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net

Milestone: 6.0.0

unsafe
stemp = UriHelper.StripBidiControlCharacters(stemp, stemp);

var hostBuilder = new ValueStringBuilder(stackalloc char[512]);
Copy link
Member Author

@MihaZupan MihaZupan Jan 8, 2021

Choose a reason for hiding this comment

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

512 is used as that is the value of the StackallocThreshold constant being introduced in #34864.

// but we shouldn't throw after the constructor.
catch (UriFormatException) { }
}
stemp = hostBuilder.ToString();
Copy link
Member Author

Choose a reason for hiding this comment

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

This allocation could be avoided, but that is being done in #34864 and this PR is not introducing new allocations.

Copy link
Contributor

@alnikola alnikola left a comment

Choose a reason for hiding this comment

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

LGTM

@MihaZupan MihaZupan merged commit 8426894 into dotnet:master Jan 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants