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

System.Text.Json: stackalloc constants + misc PR feedback #55350

Merged
merged 4 commits into from
Jul 18, 2021

Conversation

CodeBlanch
Copy link
Contributor

@ghost
Copy link

ghost commented Jul 8, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Follow-up feedback from #54186

See:

/cc @layomia

Author: CodeBlanch
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@GrabYourPitchforks
Copy link
Member

GrabYourPitchforks commented Jul 8, 2021

The pattern (stackalloc byte[...]).Slice(...) shouldn't be necessary for things which are scratch buffers and where the actual number of bytes written will be received as a variable a few lines below. In these cases, the easiest thing to do would be to stackalloc without slicing, perform the operation, then slice the scratch buffer after you know exactly how many bytes were written to it.

Edit: to see why this is, consider the pattern below.

Span<byte> theSpan = (length <= MaxStackAllocSize)
    ? stackalloc byte[MaxStackAllocSize].Slice(0, length) // will always create a span of the exact length
    : ArrayPool<byte>.Shared.Rent(length); // may return a buffer of a larger length

Since the "else" case of renting from the array pool gives a potentially larger-than-expected buffer, any code which follows already has to account for the scratch buffer not being sized exactly as the caller specified. Since the code's already accounting for this, there's no need for the Slice call in the stackalloc case. Simply stackalloc the constant amount and charge forward.

@@ -140,7 +140,7 @@ private void WriteBase64EscapeProperty(ReadOnlySpan<char> propertyName, ReadOnly
int length = JsonWriterHelper.GetMaxEscapedLength(propertyName.Length, firstEscapeIndexProp);

Span<char> escapedPropertyName = length <= JsonConstants.StackallocThreshold ?
stackalloc char[length] :
stackalloc char[JsonConstants.StackallocThreshold] :
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we happy with using the same threshold for chars, effectivly doubling the stackalloc size? I know the thresholds are all over the place in this repo (100,256,512,1024), but I'm still a bit uneasy with 512 bytes (not even talking about 1024 in MultiPartcontent).

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if the threshold were a fixed number of bytes, scaled accordingly for chars. That said, as you say we're very inconsistent; we've talked about going through and standardizing everything across the repo, but haven't done so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to have a scaled threshold for char allocations.

@@ -38,7 +38,7 @@ public static byte[] GetEscapedPropertyNameSection(ReadOnlySpan<byte> utf8Value,
int length = JsonWriterHelper.GetMaxEscapedLength(utf8Value.Length, firstEscapeIndexVal);

Span<byte> escapedValue = length <= JsonConstants.StackallocThreshold ?
stackalloc byte[length] :
stackalloc byte[JsonConstants.StackallocThreshold] :
Copy link
Member

Choose a reason for hiding this comment

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

Was there a request to change from length to StackallocThreshold? It seems length would be more correct unless I'm missing some perf (256 faster than specific number?) or other reason.

Copy link
Member

Choose a reason for hiding this comment

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

Just part of the general "stackallocing a const value is optimized over using a variable value" work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original discussion: #54186 (comment)

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks. I rebased your changes to fix the merge conflict.

@stephentoub stephentoub merged commit f2a55e2 into dotnet:main Jul 18, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 17, 2021
@CodeBlanch CodeBlanch deleted the json-timespan-followup branch September 9, 2021 16:48
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.

6 participants