Fix OverflowException in JsonSerializer.Serialize - Part 2 #34040
Conversation
felipepessoto
requested review from
ahsonkhan,
jozkee,
layomia and
steveharter
as code owners
|
/azp run |
|
Commenter does not have sufficient privileges for PR 34040 in repo dotnet/runtime |
|
@felipepessoto - can you rebase your change on top of master? |
…d.Rent/Array constructor. Because it can behave differently in OSX, where it supports int.MaxValue array size.
felipepessoto
force-pushed
the
fix-609-p2
branch
from
3dd438f
to
9a865dd
Compare
Done |
| @@ -12,6 +12,13 @@ namespace System.Text.Json | |||
| { | |||
| internal static partial class ThrowHelper | |||
| { | |||
| [DoesNotReturn] | |||
| [MethodImpl(MethodImplOptions.NoInlining)] | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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==
There was a problem hiding this comment.
Should we create a issue to keep track of this?
ghost
locked as resolved and limited conversation to collaborators
Explicitly throw the OutOfMemoryException instead of relying on Shared.Rent/Array constructor. Because it can behave differently in OSX, where it supports int.MaxValue array size.
Fix #609