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

Remove incorrect ExceptionArgument.length for ArgumentOutOfRangeException in MemoryMarshal #105475

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 25, 2024

The actual name of the argument is source or span therefore this is incorrect.

Arguably an ArgumentException should be thrown in these case, not ArgumentOutOfRangeException, but it would be a breaking change to change this.

Just remove the ExceptionArgument like in Span.Slice.

Relatively large diff improvements as a result:

Found 278 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 39659317
Total bytes of diff: 39654405
Total bytes of delta: -4912 (-0.01 % of base)
Total relative delta: -29.60
    diff is an improvement.
    relative diff is an improvement.
-34 (-15.32 % of base) - System.IO.Hashing.XxHash32+State:Complete(int,System.ReadOnlySpan`1[ubyte]):uint:this
 G_M56174_IG01:
        push     rbp
        mov      rbp, rsp
 						;; size=4 bbWeight=1 PerfScore 1.25
 G_M56174_IG02:
        cmp      byte  ptr [rdi+0x14], 0
        je       SHORT G_M56174_IG04
 						;; size=6 bbWeight=1 PerfScore 4.00
 G_M56174_IG03:
        mov      eax, dword ptr [rdi]
        rol      eax, 1
        mov      r8d, dword ptr [rdi+0x04]
        rol      r8d, 7
        add      eax, r8d
        mov      r8d, dword ptr [rdi+0x08]
        rol      r8d, 12
        add      eax, r8d
        mov      r8d, dword ptr [rdi+0x0C]
        rol      r8d, 18
        add      eax, r8d
        jmp      SHORT G_M56174_IG05
        align    [5 bytes for IG06]
 						;; size=44 bbWeight=0.50 PerfScore 6.62
 G_M56174_IG04:
        mov      eax, dword ptr [rdi+0x10]
 						;; size=3 bbWeight=0.50 PerfScore 1.00
 G_M56174_IG05:
        add      eax, esi
        cmp      ecx, 4
        jl       SHORT G_M56174_IG07
 						;; size=7 bbWeight=1 PerfScore 1.50
 G_M56174_IG06:
        cmp      ecx, 4
-       jl       G_M56174_IG12
+       jl       SHORT G_M56174_IG11
        imul     edi, dword ptr [rdx], 0xD1FFAB1E
        add      eax, edi
        rol      eax, 17
        imul     eax, eax, 0xD1FFAB1E
        cmp      ecx, 4
        jb       SHORT G_M56174_IG11
        add      rdx, 4
        add      ecx, -4
        cmp      ecx, 4
        jge      SHORT G_M56174_IG06
-						;; size=43 bbWeight=4 PerfScore 48.00
+						;; size=39 bbWeight=4 PerfScore 48.00
 G_M56174_IG07:
        xor      edi, edi
        test     ecx, ecx
        jle      SHORT G_M56174_IG09
-       align    [15 bytes for IG08]
-						;; size=21 bbWeight=1 PerfScore 1.75
+       align    [3 bytes for IG08]
+						;; size=9 bbWeight=1 PerfScore 1.75
 G_M56174_IG08:
        movzx    rsi, byte  ptr [rdx+rdi]
        imul     esi, esi, 0xD1FFAB1E
        add      esi, eax
        rol      esi, 11
        imul     eax, esi, 0xD1FFAB1E
        inc      edi
        cmp      edi, ecx
        jl       SHORT G_M56174_IG08
 						;; size=28 bbWeight=4 PerfScore 33.00
 G_M56174_IG09:
        mov      ecx, eax
        shr      ecx, 15
        xor      ecx, eax
        imul     eax, ecx, 0xD1FFAB1E
        mov      ecx, eax
        shr      ecx, 13
        xor      ecx, eax
        imul     eax, ecx, 0xD1FFAB1E
        mov      ecx, eax
        shr      ecx, 16
        xor      eax, ecx
 						;; size=33 bbWeight=1 PerfScore 7.00
 G_M56174_IG10:
        pop      rbp
        ret      
 						;; size=2 bbWeight=1 PerfScore 1.50
 G_M56174_IG11:
        mov      rax, 0xD1FFAB1E      ; code for System.ThrowHelper:ThrowArgumentOutOfRangeException()
        call     [rax]System.ThrowHelper:ThrowArgumentOutOfRangeException()
        int3     
 						;; size=13 bbWeight=0 PerfScore 0.00
-G_M56174_IG12:
-       mov      edi, 40
-       mov      rax, 0xD1FFAB1E      ; code for System.ThrowHelper:ThrowArgumentOutOfRangeException(int)
-       call     [rax]System.ThrowHelper:ThrowArgumentOutOfRangeException(int)
-       int3     
-						;; size=18 bbWeight=0 PerfScore 0.00
 
-; Total bytes of code 222, prolog size 4, PerfScore 105.62, instruction count 65, allocated bytes for code 222 (MethodHash=15592491) for method System.IO.Hashing.XxHash32+State:Complete(int,System.ReadOnlySpan`1[ubyte]):uint:this (FullOpts)
+; Total bytes of code 188, prolog size 4, PerfScore 105.62, instruction count 61, allocated bytes for code 188 (MethodHash=15592491) for method System.IO.Hashing.XxHash32+State:Complete(int,System.ReadOnlySpan`1[ubyte]):uint:this (FullOpts)

MihuBot/runtime-utils#557

…ception` in `MemoryMarshal`

Arguably an `ArgumentException` should be thrown in these case, not `ArgumentOutOfRangeException`, but it would be a breaking change to change this.
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 25, 2024
Copy link
Contributor

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

@stephentoub
Copy link
Member

While I know it's not perfect, I prefer it the way it is: the caller is told that the length is the problem. In an ideal world, maybe it would be span.Length, but I'd rather just leave things as they are.

@xtqqczze
Copy link
Contributor Author

While I know it's not perfect, I prefer it the way it is: the caller is told that the length is the problem. In an ideal world, maybe it would be span.Length, but I'd rather just leave things as they are.

In the case of Read<T>(ReadOnlySpan<byte> source) at least, a paramName argument with a value of length doesn't seem to resolve any ambiguity.

The documentation for ArgumentOutOfRangeException states: "The exception that is thrown when the value of an argument is outside the allowable range of values as defined by the invoked method."

In this case it is the property of the argument that is out of range. Perhaps the documentation should be updated?

@stephentoub
Copy link
Member

In the case of Read(ReadOnlySpan source) at least, a paramName argument with a value of length doesn't seem to resolve any ambiguity.

What else would the "length" be other than "source.Length"?

@xtqqczze
Copy link
Contributor Author

In the case of Read(ReadOnlySpan source) at least, a paramName argument with a value of length doesn't seem to resolve any ambiguity.

What else would the "length" be other than "source.Length"?

I mean what else would the ArgumentOutOfRangeException be in relation to. It seems unnecessary to specify the paramName at all.

@stephentoub
Copy link
Member

In the case of Read(ReadOnlySpan source) at least, a paramName argument with a value of length doesn't seem to resolve any ambiguity.

What else would the "length" be other than "source.Length"?

I mean what else would the ArgumentOutOfRangeException be in relation to. It seems unnecessary to specify the paramName at all.

It tells you what's wrong with it. The length is wrong.

@AaronRobinsonMSFT
Copy link
Member

In the case of Read(ReadOnlySpan source) at least, a paramName argument with a value of length doesn't seem to resolve any ambiguity.

What else would the "length" be other than "source.Length"?

I mean what else would the ArgumentOutOfRangeException be in relation to. It seems unnecessary to specify the paramName at all.

It tells you what's wrong with it. The length is wrong.

I tend to agree. I don't think this clarifies all that much and since it is a breaking change I don't think it is appropriate.

@xtqqczze
Copy link
Contributor Author

since it is a breaking change I don't think it is appropriate.

I hadn't considered that changing the paramName argument would be a breaking change.

But the ArgumentException class exposes a ParamName property, which unlike the exception message is not translated for localization purposes; so these changes could in fact be breaking.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants