issue-44801 EnsureCapacity Apis For List Stack Queue

[lateapexearlyspeed]

Fix #44801
Current status is to make sure there is a PR on progress, will notify when it is ready for review, thanks.

[ghost]

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

Issue Details

---

Fix #44801
Current status is to make sure there is a PR on progress, will notify when it is ready for review, thanks.

Author:	lateapexearlyspeed
Assignees:	-
Labels:	area-System.Collections
Milestone:	-

[eiriktsarpalis]

Current status is to make sure there is a PR on progress, will notify when it is ready for review, thanks.

Thanks for taking this. Consider marking this as a draft PR while working on it.

[eiriktsarpalis]

This seems out of place but there is certainly precedent elsewhere, e.g.

runtime/src/libraries/Common/src/System/Collections/Generic/EnumerableHelpers.cs

Line 57 in 1d9e50c

const int MaxArrayLength = 0x7FEFFFFF;

[stephentoub]

This can go away once #43301 lands.

[lateapexearlyspeed]

Fixed by moving this temp value to nearby usage scope.

[lateapexearlyspeed]

For Queue.EnsureCapacity(), I did not use private SetCapacity() as following check can be removed:

_tail = (_size == capacity) ? 0 : _size;

Not sure if it deserved to 'inline' most of its implementation into Queue.EnsureCapacity() rather than calling SetCapacity().

[lateapexearlyspeed]

It is ready for review now.

[eiriktsarpalis]

It seems like many of the new tests are failing: https://dev.azure.com/dnceng/public/_build/results?buildId=975051&view=ms.vss-test-web.build-test-results-tab&runId=30602816&resultId=117111&paneView=debug

Could you take a look?

[lateapexearlyspeed]

Should because of the last commit about version update which was the only one not run test in my local :( will fix later.

[lateapexearlyspeed]

@eiriktsarpalis Reopen cannot pass those failures, are they related to this PR ?

[lateapexearlyspeed]

Hi @eiriktsarpalis, build passed after last new commit (although not sure if failure is related to it, maybe it just trigger refresh build)
Could you review this commit ? It is about some improvement as commit message describes.

[eiriktsarpalis]

Hey @lateapexearlyspeed, I just pushed a commit that makes a few adjustments to your PR:

1. Ensure there is only one resize algorithm defined in each class.
2. I reverted the while (newcapacity < capacity) newcapacity *= 2; logic, since it would result in different allocation patterns in methods such as List.AddRange.
3. Added tests validating EnsureCapacity behaviour when the argument is larger than the maximum array size.
4. Fixed a bug in Stack<T> that would throw ArgumentOutOfRangeException instead of OutOfMemoryException when attempting to push more elements than the maximum array size permits.

Hope it makes sense.

[lateapexearlyspeed]

Hi @eiriktsarpalis I have no question about change, thanks.
Please note new test "EnsureCapacity_LargeCapacity_Throws" does not throw exception.

And would confirm one thing about following code in Queue.cs about overflow:

        int newcapacity = (int)((long)_array.Length * GrowFactor / 100);
        // Ensure minimum growth and account for arithmetic overflow.
        if (newcapacity < _array.Length + MinimumGrow) newcapacity = _array.Length + MinimumGrow;

In future, if GrowFactor is not 200 (for example 400), then it is possible that newcapacity is indeed overflowed after first line calculation but it skipped third line check because int newcapacity value may be larger than original newcapacity after multiple 4, for example take a 8-bit signed number: (0101 1000) << 2 = (0110 0000).
So we can accept that case as long as the calculation can finish and get value which meets MinimumGrow and requested capacity, right ?

[eiriktsarpalis]

Please note new test "EnsureCapacity_LargeCapacity_Throws" does not throw exception.

Could you elaborate?

In future, if GrowFactor is not 200 (for example 400), then it is possible that newcapacity is indeed overflowed after first line calculation but it skipped third line check because int newcapacity value may be larger than original newcapacity after multiple 4, for example take a 8-bit signed number: (0101 1000) << 2 = (0110 0000).
So we can accept that case as long as the calculation can finish and get value which meets MinimumGrow and requested capacity, right ?

GrowFactor is a constant we are unlikely to ever change. I stopped short of removing it altogether in my changes but it is essentially the same algorithm as the other two types (modulo the MinimumGrow guarantee).

EDIT: I pushed a commit that removes the GrowFactor constant altogether.

[lateapexearlyspeed]

Please note new test "EnsureCapacity_LargeCapacity_Throws" does not throw exception.

Could you elaborate?

No issue, just notice that new test failed :)

[lateapexearlyspeed]

2. I reverted the while (newcapacity < capacity) newcapacity *= 2; logic, since it would result in different allocation patterns in methods such as List.AddRange.

BTW could you help explain "different allocation pattern" ? I know it will definitely have chance to give different newcapacity result if using while (newcapacity < capacity) newcapacity *= 2;.

[eiriktsarpalis]

BTW could you help explain "different allocation pattern" ? I know it will definitely have chance to give different newcapacity result if using while (newcapacity < capacity) newcapacity *= 2;.

Calling new List<int>().AddRange(Enumerable.Range(1, 70)) allocates a buffer of size 70. OTOH calling new List<int>().EnsureCapacity(70) would allocate a buffer of size 128. The two should be consistent, however if we changed AddRange to use the same logic it would result in nontrivial increase of memory consumption.

No issue, just notice that new test failed :)

Oh. I guess it worked on my machine 😄

[eiriktsarpalis]

No issue, just notice that new test failed :)

Oh. I guess it worked on my machine 😄

Seems like a mono issue. I just checked and the following is valid in mono 6.12:

var arr = new int[int.MaxValue];
arr[int.MaxValue - 1] = 42;

So we have two options: either remove the tests or apply the SkipOnMono attribute. I'm wondering though if there's a way to only run a given test in CoreCLR. I couldn't find any attribute magic to enable such behaviour. @ViktorHofer might know if it's possible.

[ViktorHofer]

You can either use the SkipOnMonoAttribute or use the ActiveIssueAttribute, more precisely this ctor: https://github.com/dotnet/arcade/blob/1ca4a4637bf32494c8ba579c32e4512309cae751/src/Microsoft.DotNet.XUnitExtensions/src/Attributes/ActiveIssueAttribute.cs#L21

[eiriktsarpalis]

Ok, so if I wanted to have a test guaranteed to only run in CoreCLR (assuming we eventually add more entries to the TestRuntimes enum in the future), I would have to write something like [ActiveIssue("Only specifies CoreCLR behaviour", ~TestRuntimes.CoreCLR)]. Is there a more straightforward way?

[ViktorHofer]

Right, but we usually only use the ActiveIssueAttribute in cases where a behavior needs to be fixed on the excluded platform/runtime/etc. In this case it seems there's no other option so that should be fine

[eiriktsarpalis]

Thank you for your contribution!