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

Fixed error messages on collections indexing #67696

Merged
merged 3 commits into from
Apr 13, 2022
Merged

Conversation

B1Z0N
Copy link
Contributor

@B1Z0N B1Z0N commented Apr 7, 2022

Fixes #67423

About

There are multiple libraries that have ArgumentOutOfRange_Index error message in their resource files. Even if it states that

Index was out of range. Must be non-negative and less than the size of the collection.

  1. In some libraries it's used even in the situations where index being equal to the size of the collection is allowed.
  1. In some libraries it's used solely in appropriate case of "throw if index >= size". So the message is ok here.
  • System.Security.Cryptography.Xml

  • System.Security.Cryptography.Pkcs

  • System.Security.Cryptography

  • For the sake of future development of the library they were split into ArgumentOutOfRange_IndexMustBeLessThan and ArgumentOutOfRange_IndexMustBeLessThanOrEqual too. Just to be consistent throughout the repo.

  1. In some libraries it's included in resource files but never used.
  • System.Console

  • System.Diagnostics.Process

  • The same as previous.

Other

ThrowHelper

  • Some of ThrowHelper methods too were splitted into two. That's
  • ThrowArgumentOutOfRange_IndexException
  • ThrowStartIndexArgumentOutOfRange_ArgumentOutOfRange_Index

Docs

On msdn probably lots of methods with wrong Exceptions sectors. Like List<T>.IndexOf here:
image
This one actually CAN accept index that equals to it's length.

  • I think it's important to fix it too. I've created an issue, and hopefully will come to PR in dotnet-api-docs repo.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 7, 2022
@ghost
Copy link

ghost commented Apr 7, 2022

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

Issue Details

Fixes #67423

About

There are multiple libraries that have ArgumentOutOfRange_Index error message in their resource files. Even if it states that

Index was out of range. Must be non-negative and less than the size of the collection.

  1. In some libraries it's used even in the situations where index being equal to the size of the collection is allowed.
  1. In some libraries it's used solely in appropriate case of "throw if index >= size". So the message is ok here.
  • System.Security.Cryptography.Xml

  • System.Security.Cryptography.Pkcs

  • System.Security.Cryptography

  • But maybe for the sake of future development of the library we should split it into ArgumentOutOfRange_IndexMustBeLT and ArgumentOutOfRange_IndexMustBeLE too? Just to be consistent throughout the repo.

  1. In some libraries it's included in resource files but never used.
  • System.Console

  • System.Diagnostics.Process

  • Should we remove it from there, leave as is or split it like in previous question?

Other

ThrowHelper

  • Some of ThrowHelper methods too were splitted into two. That's
  • ThrowArgumentOutOfRange_IndexException
  • ThrowStartIndexArgumentOutOfRange_ArgumentOutOfRange_Index

Docs

On msdn probably lots of methods with wrong Exceptions sectors. Like List<T>.IndexOf here:
image
This one actually CAN accept index that equals to it's length.

  • I want to fix it, but I thought it would be quite a piece of a commitment, so decided to ask first: may I?
Author: B1Z0N
Assignees: -
Labels:

area-System.Collections

Milestone: -


if (length <= 0)
{
if (length == 0)
return Array.Empty<char>();
throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_Index);
throw new ArgumentOutOfRangeException(nameof(length), SR.ArgumentOutOfRange_NegativeLength);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this minor wrong error message was fixed here. Now on negative length it actually signals about negative length and not about some indexing errors.

@B1Z0N
Copy link
Contributor Author

B1Z0N commented Apr 7, 2022

/azp run dotnet-linker-tests

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 67696 in repo dotnet/runtime

@danmoseley
Copy link
Member

/azp run dotnet-linker-tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmoseley
Copy link
Member

You need write permissions to ask azp for extra configurations so I did it. Although, how are linker tests relevant here?

@B1Z0N
Copy link
Contributor Author

B1Z0N commented Apr 7, 2022

You need write permissions to ask azp for extra configurations so I did it. Although, how are linker tests relevant here?

Thanks a lot for help with rerun!

I'm kinda new contributor and just trying to sort out why this is failing. I tried getting access to any logs on errors, but haven't managed to do that. Linker error stated something about git fetch exit code 128, so I thought it might be just some internal thing and decided to retry.

Ah, and force push there was just my way to trigger rerun. Probably not the best one :)

@danmoseley
Copy link
Member

@B1Z0N gotcha. what you can do is close and reopen the PR. That's most costly though as it reruns everything from the start.

@jkoritzinsky is this familiar? obviously not related to this change.

  Starting:    LibraryImportGenerator.Unit.Tests (parallel test collections = on, max threads = 4)
    LibraryImportGenerator.UnitTests.Compiles.ValidateSnippetsWithMarshalType [SKIP]
      No current scenarios to test.
Fatal error. System.AccessViolationException: Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
   at System.Reflection.Metadata.MetadataReader.InitializeTableReaders(System.Reflection.Internal.MemoryBlock, System.Reflection.Metadata.Ecma335.HeapSizes, Int32[], Int32[])
   at System.Reflection.Metadata.MetadataReader..ctor(Byte*, Int32, System.Reflection.Metadata.MetadataReaderOptions, System.Reflection.Metadata.MetadataStringDecoder, System.Object)
   at System.Reflection.Metadata.PEReaderExtensions.GetMetadataReader(System.Reflection.PortableExecutable.PEReader, System.Reflection.Metadata.MetadataReaderOptions, System.Reflection.Metadata.MetadataStringDecoder)
   at Microsoft.CodeAnalysis.PEModule.InitializeMetadataReader()
   at Microsoft.CodeAnalysis.PEModule.get_MetadataReader()
   at Microsoft.CodeAnalysis.PEModule.GetMetadataModuleNamesOrThrow()
   at Microsoft.CodeAnalysis.AssemblyMetadata.GetOrCreateData()
   at Microsoft.CodeAnalysis.AssemblyMetadata.GetModules()
   at Microsoft.CodeAnalysis.AssemblyMetadata.IsValidAssembly()

Separated "Must be less than the size" and "Must be less than or equal to the size" error messages.
Now first one goes in "throw on index >= size case" and the second goes in "throw on index > size".

Fix dotnet#67423
@B1Z0N
Copy link
Contributor Author

B1Z0N commented Apr 11, 2022

Hi! It's been a few days since I've pushed changes. May someone take a look please? Or point me into if I'm missing something. Thanks for your time.

@marek-safar @MichalStrehovsky @danmoseley

@@ -42,7 +42,7 @@ public void CopyTo(Array array!!, int index)
throw new ArgumentOutOfRangeException(nameof(index), SR.ArgumentOutOfRange_NeedNonNegNum);

if (array.Length - index < this.Count)
throw new ArgumentException(SR.ArgumentOutOfRange_Index, nameof(index));
throw new ArgumentException(SR.ArgumentOutOfRange_IndexMustBeLessOrEqual, nameof(index));
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, although I see Arg_ArrayPlusOffTooSmall is used elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Oh, nm you're just renaming

@marek-safar
Copy link
Contributor

@danmoseley you looked into this already could you approve?

@danmoseley
Copy link
Member

Yeah, reading through it now..

@danmoseley
Copy link
Member

Looks good to me with a couple comments?
TBH, in many of these cases, neither message is ideal because eg the count passed is also relevant. But they aren't getting worse.

Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

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

thanks @B1Z0N

@B1Z0N
Copy link
Contributor Author

B1Z0N commented Apr 13, 2022

@danmoseley these errors seem to be unrelated as they are present in other PR builds. What should I do now?🤔

@danmoseley danmoseley merged commit a9b3898 into dotnet:main Apr 13, 2022
@danmoseley
Copy link
Member

Thanks. Perhaps you're interested in another contribution?

@B1Z0N
Copy link
Contributor Author

B1Z0N commented Apr 13, 2022

Actually, pretty much interested. I like how the this community contribution process is arranged. Thanks to you, including. Why are you asking😅?

@danmoseley
Copy link
Member

Just because I appreciate and enjoy working with new contributors and hope we can grow our community here.
The main bottleneck is generally code reviews. There is a certain number of committers who can review. As contributors get more familiar with working in the repo, reviews tend to get quicker. Either way smaller changes are easier to review and can often flow much faster than larger ones.

@AndyAyersMS
Copy link
Member

We're seeing some perf regressions that might be a result of this change: dotnet/perf-autofiling-issues#4738, eg:

newplot - 2022-04-22T110749 466

Looking at the change I don't have a good explanation as to why it might cause these specific regressions, so more investigation is needed.

See also #68338.

if (startIndex > value.Length - 1)
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.startIndex, ExceptionResource.ArgumentOutOfRange_Index); // differs from other overloads, which throw base ArgumentException
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.startIndex, ExceptionResource.ArgumentOutOfRange_IndexMustBeLess);
if (startIndex >= value.Length)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi 👋

Probably, dotnet/perf-autofiling-issues#4738 and #68338 are my bad. Here I've changed to >= so that it'd be more consistent(>=, ...IndexMustBeLess) and more readable.

But after seeing this benchmark and being able to reproduce it, I've reverted my commit and compared it's performance to upstream. I've found out that it's my commit that causing this and somehow, on jit asm level >= approx two times slower then > len - 1. So I've fixed it and got the benchmark result:

Method Job Toolchain Mean Error StdDev Median Min Max Ratio RatioSD Allocated Alloc Ratio
ReadStructAndReverseBE Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 11.345 ns 0.1365 ns 0.1277 ns 11.394 ns 11.116 ns 11.477 ns 0.98 0.01 - NA
ReadStructAndReverseBE Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 11.566 ns 0.0608 ns 0.0539 ns 11.568 ns 11.472 ns 11.660 ns 1.00 0.00 - NA
ReadStructAndReverseLE Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 1.992 ns 0.0726 ns 0.0679 ns 1.981 ns 1.831 ns 2.133 ns 1.01 0.03 - NA
ReadStructAndReverseLE Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 1.983 ns 0.0088 ns 0.0078 ns 1.984 ns 1.962 ns 1.994 ns 1.00 0.00 - NA
ReadStructFieldByFieldBE Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 10.517 ns 0.5510 ns 0.5896 ns 10.242 ns 9.892 ns 12.019 ns 1.00 0.07 - NA
ReadStructFieldByFieldBE Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 10.478 ns 0.4902 ns 0.5645 ns 10.132 ns 9.623 ns 11.228 ns 1.00 0.00 - NA
ReadStructFieldByFieldLE Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 9.229 ns 0.0381 ns 0.0337 ns 9.227 ns 9.180 ns 9.296 ns 1.01 0.00 - NA
ReadStructFieldByFieldLE Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 9.139 ns 0.0066 ns 0.0055 ns 9.136 ns 9.133 ns 9.153 ns 1.00 0.00 - NA
ReadStructFieldByFieldUsingBitConverterLE Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 19.257 ns 0.1194 ns 0.0997 ns 19.255 ns 19.008 ns 19.404 ns 0.90 0.01 - NA
ReadStructFieldByFieldUsingBitConverterLE Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 21.446 ns 0.1205 ns 0.1127 ns 21.434 ns 21.258 ns 21.603 ns 1.00 0.00 - NA
ReadStructFieldByFieldUsingBitConverterBE Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 30.687 ns 0.0825 ns 0.0732 ns 30.695 ns 30.544 ns 30.804 ns 0.98 0.00 - NA
ReadStructFieldByFieldUsingBitConverterBE Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 31.372 ns 0.1749 ns 0.1636 ns 31.297 ns 31.221 ns 31.792 ns 1.00 0.00 - NA
MeasureReverseEndianness Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 402.172 ns 0.5438 ns 0.5086 ns 402.114 ns 401.325 ns 402.919 ns 0.91 0.00 - NA
MeasureReverseEndianness Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 441.049 ns 2.4377 ns 2.2803 ns 440.656 ns 438.690 ns 445.914 ns 1.00 0.00 - NA
MeasureReverseUsingNtoH Job-FQAXKT /runtime.fixed/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 421.494 ns 1.1445 ns 0.9557 ns 421.163 ns 420.339 ns 423.134 ns 1.01 0.03 - NA
MeasureReverseUsingNtoH Job-KKCPCD /runtime/artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/7.0.0/corerun 423.682 ns 9.7031 ns 11.1741 ns 428.362 ns 407.732 ns 437.481 ns 1.00 0.00 - NA

Also see here the difference between a >= b and a > b - 1 in jit asm.

Mainly this diff:

// a >= b case
    L0012: cmp esi, eax
    L0014: setge cl

// a > b - 1 case
    L0012: dec eax
    L0013: cmp esi, eax
    L0015: setg cl

Now the questions are

  1. Should I open another PR with quickfix, or how is it supposed to be done?
  2. Maybe I should investigate why it's faster, is it so on most platforms? And try to put that as optimization into jit. Or is it too minor/unreliable/inconsistent throughout platforms/etc?

Copy link
Member

Choose a reason for hiding this comment

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

  • Should I open another PR with quickfix, or how is it supposed to be done?

Yes, open a new PR.

  • Maybe I should investigate why it's faster, is it so on most platforms? And try to put that as optimization into jit. Or is it too minor/unreliable/inconsistent throughout platforms/etc?

You're certainly welcome to investigate.

It can be tricky to root cause these. I would hazard a guess the perf issue is not directly related to the change above but some other microarchitectural issue (say branch alignment). Though given that we're also seeing impact on arm64 perhaps it is something directly related after all.

@dakersnar
Copy link
Contributor

dakersnar commented Apr 27, 2022

Sounds like this is already being addressed, but here are some arm64 regressions from this change: dotnet/perf-autofiling-issues#4727

@AndyAyersMS
Copy link
Member

Also regressions from alpine x64: dotnet/perf-autofiling-issues#4870

@dakersnar
Copy link
Contributor

Another regression on Windows x86: dotnet/perf-autofiling-issues#4706

@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections 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.

List<T>.IndexOf outputs incorrect error message
5 participants