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

MemoryPool<byte>.Shared.MaxBufferSize seems incorrect #61560

Closed
Tracked by #64598
rwv37 opened this issue Nov 14, 2021 · 6 comments · Fixed by #61755
Closed
Tracked by #64598

MemoryPool<byte>.Shared.MaxBufferSize seems incorrect #61560

rwv37 opened this issue Nov 14, 2021 · 6 comments · Fixed by #61755

Comments

@rwv37
Copy link

rwv37 commented Nov 14, 2021

Description

I've found that System.MemoryPool.Shared.MaxBufferSize seems to be incorrect (at least on my computer). It claims that the max is int.MaxValue, but really the limit seems to be 56 bytes less than that. I believe that this is probably related to the fact that the same maximum seems to apply to the length of a directly-newed-up byte array (again, at least on my computer).

This is using .Net 5 on a Windows 10 computer with plenty of memory. I have not tried any other versions of .Net or any other computers.

Reproduction Steps

using System;
using System.Buffers;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            int value = int.MaxValue;
            TryArrayThenRental(value);
            value -= 56;
            TryArrayThenRental(value);
            ++value;
            TryArrayThenRental(value);
        }

        private static void TryArrayThenRental(int value)
        {
            bool isOK = ArrayIsOK(value);
            bool rentalIsOK = RentalIsOK(value);
            int rentalMax = MemoryPool<byte>.Shared.MaxBufferSize;
            int diff = rentalMax - value;

            Console.WriteLine($"{value}: Array: {isOK}; Rental: {rentalIsOK}; Rental Max: {rentalMax}; Diff: {diff}");
        }

        private static bool RentalIsOK(int value)
        {
            System.GC.Collect();

            MemoryPool<byte> pool = MemoryPool<byte>.Shared;
            IMemoryOwner<byte> owner = null;

            try
            {
                owner = pool.Rent(value);
            }
            catch
            {
                return false;
            }
            finally
            {
                owner?.Dispose();
            }

            return true;
        }

        private static bool ArrayIsOK(int value)
        {
            System.GC.Collect();

            try
            {
                var bytes = new byte[value];
            }
            catch
            {
                return false;
            }

            return true;
        }
    }
}

Expected behavior

I expect that the rental should succeed in all cases when I supply a number less than MaxBufferSize. I imagine (but don't know for sure that I should expect) that the raw array allocation should work if and only if the rental of the same size works. So, I expect the output to be the following, with the possible exception that "Array: xxx" might in some cases be False rather than True.

2147483647: Array: True; Rental: True; Rental Max: 2147483647; Diff: 0
2147483591: Array: True; Rental: True; Rental Max: 2147483647; Diff: 56
2147483592: Array: True; Rental: True; Rental Max: 2147483647; Diff: 55

Actual behavior

Instead, both the rental and the new byte[] fail unless the length is at most 56 bytes less than MaxBufferSize:

2147483647: Array: False; Rental: False; Rental Max: 2147483647; Diff: 0
2147483591: Array: True; Rental: True; Rental Max: 2147483647; Diff: 56
2147483592: Array: False; Rental: False; Rental Max: 2147483647; Diff: 55

Regression?

Sorry, I have no idea.

Known Workarounds

I dunno, manually make sure you're trying within the experimentally-determined "real" limit, I guess?

Configuration

  • .NET 5
  • Windows 10 Professional version 20H2 OS build 19042.1348
  • x64
  • No idea whether or not it's specific to this configuration
  • Not using Blazor

Other information

As mentioned above, I believe that this is probably related to the fact that the same maximum seems to apply to the length of a directly-newed-up byte array (again, at least on my computer).

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Nov 14, 2021
@ghost
Copy link

ghost commented Nov 14, 2021

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

Issue Details

Description

I've found that System.MemoryPool.Shared.MaxBufferSize seems to be incorrect (at least on my computer). It claims that the max is int.MaxValue, but really the limit seems to be 56 bytes less than that. I believe that this is probably related to the fact that the same maximum seems to apply to the length of a directly-newed-up byte array (again, at least on my computer).

This is using .Net 5 on a Windows 10 computer with plenty of memory. I have not tried any other versions of .Net or any other computers.

Reproduction Steps

using System;
using System.Buffers;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            int value = int.MaxValue;
            TryArrayThenRental(value);
            value -= 56;
            TryArrayThenRental(value);
            ++value;
            TryArrayThenRental(value);
        }

        private static void TryArrayThenRental(int value)
        {
            bool isOK = ArrayIsOK(value);
            bool rentalIsOK = RentalIsOK(value);
            int rentalMax = MemoryPool<byte>.Shared.MaxBufferSize;
            int diff = rentalMax - value;

            Console.WriteLine($"{value}: Array: {isOK}; Rental: {rentalIsOK}; Rental Max: {rentalMax}; Diff: {diff}");
        }

        private static bool RentalIsOK(int value)
        {
            System.GC.Collect();

            MemoryPool<byte> pool = MemoryPool<byte>.Shared;
            IMemoryOwner<byte> owner = null;

            try
            {
                owner = pool.Rent(value);
            }
            catch
            {
                return false;
            }
            finally
            {
                owner?.Dispose();
            }

            return true;
        }

        private static bool ArrayIsOK(int value)
        {
            System.GC.Collect();

            try
            {
                var bytes = new byte[value];
            }
            catch
            {
                return false;
            }

            return true;
        }
    }
}

Expected behavior

I expect that the rental should succeed in all cases when I supply a number less than MaxBufferSize. I imagine (but don't know for sure that I should expect) that the raw array allocation should work if and only if the rental of the same size works. So, I expect the output to be the following, with the possible exception that "Array: xxx" might in some cases be False rather than True.

2147483647: Array: True; Rental: True; Rental Max: 2147483647; Diff: 0
2147483591: Array: True; Rental: True; Rental Max: 2147483647; Diff: 56
2147483592: Array: True; Rental: True; Rental Max: 2147483647; Diff: 55

Actual behavior

Instead, both the rental and the new byte[] fail unless the length is at most 56 bytes less than MaxBufferSize:

2147483647: Array: False; Rental: False; Rental Max: 2147483647; Diff: 0
2147483591: Array: True; Rental: True; Rental Max: 2147483647; Diff: 56
2147483592: Array: False; Rental: False; Rental Max: 2147483647; Diff: 55

Regression?

Sorry, I have no idea.

Known Workarounds

I dunno, manually make sure you're trying within the experimentally-determined "real" limit, I guess?

Configuration

  • .NET 5
  • Windows 10 Professional version 20H2 OS build 19042.1348
  • x64
  • No idea whether or not it's specific to this configuration
  • Not using Blazor

Other information

As mentioned above, I believe that this is probably related to the fact that the same maximum seems to apply to the length of a directly-newed-up byte array (again, at least on my computer).

Author: rwv37
Assignees: -
Labels:

area-System.Buffers, untriaged

Milestone: -

teo-tsirpanis added a commit to teo-tsirpanis/dotnet-runtime that referenced this issue Nov 17, 2021
`ArrayMemoryPool.MaxBufferSize`'s value was changed to `Array.MaxLength`.
@GrabYourPitchforks GrabYourPitchforks added bug and removed untriaged New issue has not been triaged by the area owner labels Nov 18, 2021
@GrabYourPitchforks GrabYourPitchforks modified the milestone: 7.0.0 Nov 18, 2021
@GrabYourPitchforks
Copy link
Member

I expect that the rental should succeed in all cases when I supply a number less than MaxBufferSize.

That assumption is not valid. Per MSDN:

This property represents a runtime limitation, the maximum number of elements (not bytes) the runtime will allow in an array. There is no guarantee that an allocation under this length will succeed, but all attempts to allocate a larger array will fail.

If you look at consumers of Array.MaxLength, they tend to use it as a clamp rather than triggering outright failure if the requested element count is too large. For example, ArrayPool<T>.Shared doesn't look at this property; it just attempts the allocation anyway and lets the runtime fail the operation with OOM.

I'm torn on whether we should change the implementation of this property, considering that int.MaxValue is always a legal (if pointless) result. If we do change it, making it equal to Array.MaxValue (as you had implied) seems like the correct path.

@rwv37
Copy link
Author

rwv37 commented Nov 18, 2021

Having a property to say that "this int must be less than or equal to the maximum possible value of an int, but that's not a promise that any int less than or equal to it will work" seems, as you said, pointless. Why not have it on every interface? "The length of a string is an int. That int must be less than or equal to the maximum possible value of an int. However, that's not to say that the length of a string can be that long. In fact there is no guarantee that there is any int that is a valid length for a string. Not even zero."

Its mere existence is misleading even if it is technically correct.

@GrabYourPitchforks
Copy link
Member

Why not have it on every interface?

It's an abstract member of the base class, and there are legitimate reasons for an implementation to say "I have constraints." aspnet's pool is a prime example.

https://github.com/dotnet/aspnetcore/blob/4226c40df06f1e2edc4576ea2b1c787d69238bb4/src/Shared/Buffers.MemoryPool/PinnedBlockMemoryPool.cs#L13-L24

The ArrayMemoryPool<T> implementation is essentially limitless. So using int.MaxValue to say "I am unconstrained" does make sense, even if it's not technically correct due to runtime limitations.

@rwv37
Copy link
Author

rwv37 commented Nov 18, 2021

Yes, I understand that there are legitimate reasons for an implementation to say that it has constraints. What I guess I don't understand is why this implementation doesn't. It seems to me that the implementation is not limitless. That some theoretical different implementation might have a greater limit seems like it's neither here nor there with respect to that.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 13, 2021
@jeffhandley jeffhandley added this to the 7.0.0 milestone Feb 1, 2022
@ghost ghost added in-pr There is an active PR which will close this issue when it is merged and removed in-pr There is an active PR which will close this issue when it is merged labels Jun 1, 2022
jeffhandley pushed a commit that referenced this issue Jun 24, 2022
`ArrayMemoryPool.MaxBufferSize`'s value was changed to `Array.MaxLength`.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants