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

Jump to bottom

Remove excess allocations in Uri.ReCreateParts #34864

Merged
merged 13 commits into from
Jan 22, 2021
Merged

Remove excess allocations in Uri.ReCreateParts #34864

merged 13 commits into from
Jan 22, 2021

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Apr 12, 2020 •
edited
Loading

Fixes #22903

For a sample Uri containing non-ascii chars, allocations for fetching PathAndQuery and IdnHost are reduced from 1608 to 272 bytes (168 after the first access since the Uri is fully parsed at that point).

After this PR and #32552, it should be fairly simple to move more unsafe code to use Span.

@MihaZupan MihaZupan added this to the 5.0 milestone Apr 12, 2020
@MihaZupan MihaZupan requested review from stephentoub and a team April 12, 2020 11:23
@ghost
Copy link

ghost commented Apr 12, 2020

Tagging @dotnet/ncl as an area owner. If you would like to be tagged for a label, please notify danmosemsft.

MihaZupan
MihaZupan commented Apr 12, 2020
View reviewed changes
src/libraries/System.Private.Uri/src/System/UriHelper.cs

static void EnsureCapacity(char[]? dest, int destSize, int requiredSize)
Copy link
Member Author

@MihaZupan MihaZupan Apr 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This looks like a bug in the current implementation as the param is not ref char[], therefore the resize isn't visible.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need a test that would catch it?

Copy link
Member Author

Choose a reason for hiding this comment

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

The overload accepting a char[] destination was only called from ReCreateParts, after this conservative worst-case capacity calculation. I believe it was thankfully unreachable code, as we never needed a resize.

MihaZupan
MihaZupan commented Apr 12, 2020
View reviewed changes
src/libraries/System.Private.Uri/src/System/UriHelper.cs Outdated
vsb.TryCopyTo(dest.AsSpan(destPos), out int charsWritten);
destPos += charsWritten;
return dest;
ReadOnlySpan<bool> noEscapeCopy = MemoryMarshal.CreateReadOnlySpan(ref MemoryMarshal.GetReference(noEscape), UnreservedReservedTable.Length);
Copy link
Member Author

@MihaZupan MihaZupan Apr 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a hack to avoid code analysis from preventing us passing the noEscape span as a parameter with

CS8352 Cannot use local 'noEscape' in this context because it may expose referenced variables outside of their declaration scope
and
CS8350 This combination of arguments to 'UriHelper.EscapeStringToBuilder(ReadOnlySpan<char>, ref ValueStringBuilder, ReadOnlySpan<bool>, bool)' is disallowed because it may expose variables referenced by parameter 'noEscape' outside of their declaration scope

Is this still legal from the runtime's perspective? Can it be avoided (without duplicating EscapeToBuilder by inlining it into this method)?
@stephentoub @jkotas

Copy link
Member

Choose a reason for hiding this comment

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

It is ok from the runtime's perspective.

The only way to avoid this today that I think of is to stackalloc the flags in the same method that they are used in.

MihaZupan
MihaZupan commented Apr 12, 2020
View reviewed changes
src/libraries/System.Private.Uri/src/System/Uri.cs
copy.Append(dest.AsSpan(start, dest.Length - start));

dest.Length = start;
ReadOnlySpan<char> copySpan = MemoryMarshal.CreateReadOnlySpan(ref copy.GetPinnableReference(), copy.Length);
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as #34864 (comment) for using this instead of copy.AsSpan()

@MihaZupan MihaZupan mentioned this pull request Apr 13, 2020
Closed
stephentoub
stephentoub reviewed Apr 16, 2020
View reviewed changes
src/libraries/System.Private.Uri/src/System/Uri.cs Outdated
}
}
else if (_syntax.InFact(UriSyntaxFlags.ConvertPathSlashes) && InFact(Flags.BackslashInPath))
{
for (int i = pos; i < end; ++i)
Span<char> chars = dest.RawChars.Slice(start, dest.Length - start);
for (int i = 0; i < chars.Length; i++)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this change. Why are we slicing to get chars and iterating over its length, but then using dest? Assuming that's a bug and dest should be chars, seems like we then have a test hole?

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, this should be chars. I will look into getting a test for this

Copy link
Member Author

@MihaZupan MihaZupan Apr 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This block is currently unreachable as we always add the ShouldBeCompressed flag when we have ConvertPathSlashes. That looks like I bug - I'll look into cases where the output is changed because of this (could be this is okay and this block can simply be removed).

@jaredpar jaredpar mentioned this pull request Apr 20, 2020
Closed
@karelz
Copy link
Member

karelz commented Jul 17, 2020

Looks like all feedback is addressed now. @stephentoub can you please re-review so that we can merge? Thanks!

@karelz
Copy link
Member

karelz commented Jul 17, 2020 •
edited
Loading

@alnikola @wfurt @scalablecory can you please review it as Stephen is OOF?

@karelz karelz modified the milestones: 5.0.0, 6.0.0 Aug 31, 2020
@stephentoub stephentoub reopened this Sep 14, 2020
@stephentoub
Copy link
Member

@MihaZupan, can you rebase this to address the conflicts? Thanks.

@MihaZupan MihaZupan closed this Oct 6, 2020
@MihaZupan MihaZupan reopened this Oct 6, 2020
@MihaZupan
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MihaZupan
Copy link
Member Author

Closing as not actionable until reviewed further.
I will try to get someone to review the PR.

@MihaZupan MihaZupan closed this Dec 14, 2020
@karelz
Copy link
Member

karelz commented Dec 15, 2020

Team discussion:

  • No point in splitting it up
  • We should have 2 reviewers as it is sensitive -- in January (@alnikola @stephentoub?)

@alnikola alnikola reopened this Jan 4, 2021
@alnikola
Copy link
Contributor

alnikola commented Jan 4, 2021 •
edited
Loading

This PR is being under review now.

This was referenced Jan 8, 2021
Merged
Merged
alnikola
alnikola approved these changes Jan 21, 2021
View reviewed changes
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
Copy link
Member Author

@stephentoub Could you PTAL at the changes now that this PR doesn't include changes from #46747 and #46751?

stephentoub
stephentoub reviewed Jan 22, 2021
View reviewed changes
src/libraries/System.Private.Uri/src/System/Uri.cs
copy.Append(dest.AsSpan(start, dest.Length - start));

dest.Length = start;
ReadOnlySpan<char> copySpan = MemoryMarshal.CreateReadOnlySpan(ref copy.GetPinnableReference(), copy.Length);
Copy link
Member

Choose a reason for hiding this comment

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

How is this different from copy.AsSpan()?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see your GitHub comment on a similar line below. It'd be worth a comment in the code.

Copy link
Member Author

@MihaZupan MihaZupan Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Added comments.
We should be able to Spanify more Uri code now and hopefully remove these hacks in the future.

@runfoapp runfoapp bot mentioned this pull request Jan 22, 2021
Closed
@MihaZupan MihaZupan merged commit 39d72d4 into dotnet:master Jan 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas left review comments

@alnikola alnikola alnikola approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees

@MihaZupan MihaZupan

Labels
area-System.Net
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

[Uri] Uri.ReCreateParts allocates a char[] that could be avoided
5 participants
@MihaZupan @stephentoub @karelz @alnikola @jkotas