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

[C#] Reduce allocations when using ArrayPool #34636

Closed
ovska opened this issue Mar 19, 2023 · 0 comments · Fixed by #39166
Closed

[C#] Reduce allocations when using ArrayPool #34636

ovska opened this issue Mar 19, 2023 · 0 comments · Fixed by #39166

Comments

@ovska
Copy link

ovska commented Mar 19, 2023

Describe the enhancement requested

Quick single-use buffers are rented using helper methods that cause a (small) allocation on every invocation due to the delegate capturing references to the reader/writer and other related variables. This somewhat defeats the purpose of using pooled arrays, especially small ones for quick operations such as reading the message length.

Here's a reference implementation for achieving roughly the same convenience without any allocations (outside the ones the pool might make):

public readonly struct ArrayLease : IDisposable
{
    private readonly byte[] _array;
    private readonly ArrayPool<byte> _pool;

    public ArrayLease(byte[] array, ArrayPool<byte> pool)
    {
        _array = array;
        _pool = pool;
    }

    public void Dispose()
    {
        _pool.Return(_array);
    }
}

public static ArrayLease RentReturn(this ArrayPool<byte> pool, int length, out Memory<byte> buffer)
{
    byte[] array = pool.Rent(length);
    buffer = array.AsMemory(0, length);
    return new ArrayLease(array, pool);
}

Example usage:

- await Buffers.RentReturnAsync(4, async (buffer) =>
+ using (Buffers.RentReturn(4, out Memory<byte> buffer))
  {
      int footerLength;
      checked
      {
          footerLength = (int)(BaseStream.Position - offset);
      }

      BinaryPrimitives.WriteInt32LittleEndian(buffer.Span, footerLength);

      await BaseStream.WriteAsync(buffer, cancellationToken).ConfigureAwait(false);
- }).ConfigureAwait(false);
+ }

Component(s)

C#

CurtHagenlocher added a commit to CurtHagenlocher/arrow that referenced this issue Dec 10, 2023
Implement apacheGH-39144: Consistently pass CancellationToken through async calls.
CurtHagenlocher added a commit that referenced this issue Dec 10, 2023
### Rationale for this change

GH-34636 is a great suggestion for simplifying the code and making it more efficient by changing the delegate-based RentReturn pattern to a "using"-based one. As most of the affected call sites were the ones not passing CancellationToken properly, it was a good time to fix that as well.

### Are these changes tested?

This is basically a refactoring which doesn't add new functionality and so is covered by existing tests.

Closes #39144
* Closes: #34636

Authored-by: Curt Hagenlocher <curt@hagenlocher.org>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
@CurtHagenlocher CurtHagenlocher added this to the 15.0.0 milestone Dec 10, 2023
mapleFU pushed a commit to mapleFU/arrow that referenced this issue Dec 13, 2023
…39166)

### Rationale for this change

apacheGH-34636 is a great suggestion for simplifying the code and making it more efficient by changing the delegate-based RentReturn pattern to a "using"-based one. As most of the affected call sites were the ones not passing CancellationToken properly, it was a good time to fix that as well.

### Are these changes tested?

This is basically a refactoring which doesn't add new functionality and so is covered by existing tests.

Closes apache#39144
* Closes: apache#34636

Authored-by: Curt Hagenlocher <curt@hagenlocher.org>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
clayburn pushed a commit to clayburn/arrow that referenced this issue Jan 23, 2024
…39166)

### Rationale for this change

apacheGH-34636 is a great suggestion for simplifying the code and making it more efficient by changing the delegate-based RentReturn pattern to a "using"-based one. As most of the affected call sites were the ones not passing CancellationToken properly, it was a good time to fix that as well.

### Are these changes tested?

This is basically a refactoring which doesn't add new functionality and so is covered by existing tests.

Closes apache#39144
* Closes: apache#34636

Authored-by: Curt Hagenlocher <curt@hagenlocher.org>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
…39166)

### Rationale for this change

apacheGH-34636 is a great suggestion for simplifying the code and making it more efficient by changing the delegate-based RentReturn pattern to a "using"-based one. As most of the affected call sites were the ones not passing CancellationToken properly, it was a good time to fix that as well.

### Are these changes tested?

This is basically a refactoring which doesn't add new functionality and so is covered by existing tests.

Closes apache#39144
* Closes: apache#34636

Authored-by: Curt Hagenlocher <curt@hagenlocher.org>
Signed-off-by: Curt Hagenlocher <curt@hagenlocher.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants