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

Provide a way to reset an ArrayBufferWriter without clearing the underlying memory #49134

Closed
john-h-k opened this issue Mar 4, 2021 · 10 comments · Fixed by #88009
Closed

Provide a way to reset an ArrayBufferWriter without clearing the underlying memory #49134

john-h-k opened this issue Mar 4, 2021 · 10 comments · Fixed by #88009
Labels
api-approved API was approved in API review, it can be implemented area-System.Buffers
Milestone

Comments

@john-h-k
Copy link
Contributor

john-h-k commented Mar 4, 2021

Background and Motivation

Currently there is no way to reset an ArrayBufferWriter<T> without zeroing the underlying memory, which can be expensive and an entirely unnecessary cost often.

Proposed API

namespace System.Buffers
{
    public class ArrayBufferWriter<T> : IBufferWriter<T>
    {
+        public void Reset();
    }
}

Usage Examples

When building short-lived buffers in high performance scenarios (for example, to generate command buffers to be sent over PCIE or network to control physical devices), being able to quickly reset the buffer writer is helpful, and zeroing is an unnecessary expense.

Alternative Designs

It could alternatively be an overload of Clear:

public void Clear(bool zeroBackingArray);

Risks

Types which contain references should probably still be cleared to allow collection.

@john-h-k john-h-k added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 4, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Buffers untriaged New issue has not been triaged by the area owner labels Mar 4, 2021
@ghost
Copy link

ghost commented Mar 4, 2021

Tagging subscribers to this area: @tannergooding, @pgovind, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

Currently there is no way to reset an ArrayBufferWriter<T> without zeroing the underlying memory, which can be expensive and an entirely unnecessary cost often.

Proposed API

namespace System.Buffers
{
    public class ArrayBufferWriter<T> : IBufferWriter<T>
    {
+        public void Reset();
    }
}

Usage Examples

When building short-lived buffers in high performance scenarios (for example, to generate command buffers to be sent over PCIE or network to control physical devices), being able to quickly reset the buffer writer is helpful, and zeroing is an unnecessary expense.

Alternative Designs

It could alternatively be an overload of Clear:

public void Clear(bool zeroBackingArray);

Risks

Types which contain references should probably still be cleared to allow collection.

Author: john-h-k
Assignees: -
Labels:

api-suggestion, area-System.Buffers, untriaged

Milestone: -

@geeknoid
Copy link
Member

I was just coming here to make exactly the same suggestion. We end up having a custom implementation of ArrayBufferWriter in our project which uses a Reset method. It'd be great if we didn't need that.

@GrabYourPitchforks
Copy link
Member

@tannergooding @pgovind any aversion to marking this ready for review? Seems like a straightforward enough proposal.

/cc @davidfowl in case you'd find it interesting in your projects.

@stephentoub
Copy link
Member

stephentoub commented Apr 22, 2021

I have to ask... do we even need a new API? Could we instead just retcon the design:
stephentoub@5a62634
and say that "clearing" is just like List<T>.Clear semantics, which only zeros if it contains a reference? And if we did that, does that cover enough ground that we could avoid the additional (potentially confusing Reset vs Clear) API?

@GrabYourPitchforks
Copy link
Member

In theory this could be a behavioral break, where people clear an array writer, populate some sparse elements, then advance it again, assuming that all untouched elements will have a default(T) value. Thinking back to #30649. But this is slightly different than that issue, where here we'd not be populating the underlying array with arbitrary uninitialized data. The data would still be initialized by virtue of the fact that it was supplied by the previous caller.

So maybe the theoretical break isn't a huge concern? We could always make the change now since it's early preview and monitor feedback.

@geeknoid
Copy link
Member

geeknoid commented Apr 22, 2021

@stephentoub Yeah, the implementation of my Reset function in fact has:

public void Reset()
{
#if NETFW_OR_NETSTD2
    _buffer.AsSpan(0, WrittenCount).Clear();
#else
    if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
    {
        _buffer.AsSpan(0, WrittenCount).Clear();
    }
#endif

    WrittenCount = 0;
}

But given that Clear has been systematically setting the array to 0, it is definitely a breaking change to stop doing that for primitive types.

@geeknoid
Copy link
Member

The difference with List.Clear is that the unused portion of the array inside of List is not visible to the user of the abstraction. The array underlying ArrayBufferWriter is visible to the outside world

@tannergooding
Copy link
Member

@GrabYourPitchforks, I don't have an issue here. However, if we can retcon or overload the existing design like Stephen suggested, that would be better 😄

@GrabYourPitchforks
Copy link
Member

How about we mark this ready-for-review, bring it to API review, and discuss the compat concerns there regarding the behavioral change? Either collectively we can say "new API approved!" or "let's repurpose the existing API." Either way it's forward progress on this issue.

@tannergooding tannergooding added this to the Future milestone Jul 12, 2021
@tannergooding tannergooding removed the untriaged New issue has not been triaged by the area owner label Jul 12, 2021
@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Sep 1, 2022
@stephentoub stephentoub modified the milestones: Future, 8.0.0 Jun 6, 2023
@terrajobst
Copy link
Member

terrajobst commented Jun 6, 2023

Video

  • Let's go with ResetWrittenCount()
namespace System.Buffers;

public partial class ArrayBufferWriter<T> : IBufferWriter<T>
{
    public void ResetWrittenCount();
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 6, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 24, 2023
@tannergooding tannergooding modified the milestones: 8.0.0, Future Jul 24, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 11, 2023
@adamsitnik adamsitnik modified the milestones: Future, 8.0.0 Aug 11, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Buffers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants