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

Fix OverflowException in JsonSerializer.Serialize - Part 2 #34040

Merged
merged 1 commit into from
Mar 31, 2020
Merged

Fix OverflowException in JsonSerializer.Serialize - Part 2 #34040

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PooledByteBufferWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ private void CheckAndResizeBuffer(int sizeHint)
newSize = currentLength + sizeHint;
if ((uint)newSize > int.MaxValue)
{
newSize = int.MaxValue;
ThrowHelper.ThrowOutOfMemoryException_BufferMaximumSizeExceeded((uint)newSize);
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,13 @@ namespace System.Text.Json
{
internal static partial class ThrowHelper
{
[DoesNotReturn]
[MethodImpl(MethodImplOptions.NoInlining)]
Copy link
Member

Choose a reason for hiding this comment

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

Such NoInlinings aren't necessary on .NET Core, and could make things worse. I assume these are here for the NuGet package builds? Not for this change, as you're just following the rest of these, but it seems like we should subsequently remove all of them.
cc: @steveharter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just out of curiosity, there is any issue/docs about it? I'd like to know more about it.
If I remember correctly the reason to use ThrowHelper was to reduce code size

Copy link
Member

Choose a reason for hiding this comment

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

The JIT in dotnet/runtime won't inline a method like this that always throws, anyway. But marking it as NoInlinining means that the JIT won't even look at the method implementation from the caller, which means it also won't see that the method never returns, which means the call site ends up accounting for the fact that it may, unnecessarily.
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIAYACY8gOgCUBXAOwwEt8YLAMIR8AB14AbGFADKMgG68wMXAG4aNYgGYmpBkIYBvGgzNNdxFAwCy5ABQRgAKxhgMDCAEpT5k9XNAhl4AMwZHYNwGLg5JSS8GABUACygIAHdcAHVeDGSIDgwAOQgASS5JXi4qgHN7Lw0AoLNmAE5HBt8zAF8uhj6dJmsbUkcXNw9vPv9ms1DwiEjo2Pik1Izs3OSS8srqrjrOpua2jsbA3uP+q7EoXgVsDBgmciQhtbTMnLyC4rKKqq1eoMAC8AD4GHlPtEYOkGABRBAqMR8CBcerncx9ADaNhgPwAJqVxJJ7HjCcSxJIAPIo3ho3AsHYA/Y1LwAXT6t3uj2ezDeVg+G2+23+eyBCXBkPWcK4sIRSJgdLRGJo3SAA==

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we create a issue to keep track of this?

public static void ThrowOutOfMemoryException_BufferMaximumSizeExceeded(uint capacity)
{
throw new OutOfMemoryException(SR.Format(SR.BufferMaximumSizeExceeded, capacity));
}

[DoesNotReturn]
[MethodImpl(MethodImplOptions.NoInlining)]
public static void ThrowArgumentException_DeserializeWrongType(Type type, object value)
Expand Down