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

[API Proposal]: InitialCapacity for CborWriter #91978

Closed
vcsjones opened this issue Sep 13, 2023 · 3 comments · Fixed by #92538
Closed

[API Proposal]: InitialCapacity for CborWriter #91978

vcsjones opened this issue Sep 13, 2023 · 3 comments · Fixed by #92538
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Formats.Cbor
Milestone

Comments

@vcsjones
Copy link
Member

Background and motivation

The CborWriter class currently has an internal buffer that grows as data is written to the buffer.

Callers that write large CBOR documents may end up spending a significant amount time constantly re-allocating and copying the buffer as it grows. This was observed in #91969. In this scenario, 98% of the time spent was in Array.Resize.

Callers that know before hand that they are going to write a large CBOR document currently don't have an easy way to give a hint that the internal buffer should be larger than the default.

Conversely, callers that know they are going to create small CBOR documents may give a smaller-than-the-default hint to avoid over-allocating the internal buffer.

This is similar to what we did for AsnWriter except now we are doing it for CborWriter.

API Proposal

namespace System.Formats.Cbor;

public partial class CborWriter {
    public CborWriter(int initialCapacity, CborConformanceMode conformanceMode = CborConformanceMode.Strict, bool convertIndefiniteLengthEncodings = false, bool allowMultipleRootLevelValues = false);
}

API Usage

CborWriter writer = new CborWriter(initialCapacity: 10000000);
// write 1000000 bytes of stuff to the writer.

Alternative Designs

No response

Risks

No response

@vcsjones vcsjones added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Formats.Cbor labels Sep 13, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 13, 2023
@ghost
Copy link

ghost commented Sep 13, 2023

Tagging subscribers to this area: @dotnet/area-system-formats-cbor, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

The CborWriter class currently has an internal buffer that grows as data is written to the buffer.

Callers that write large CBOR documents may end up spending a significant amount time constantly re-allocating and copying the buffer as it grows. This was observed in #91969. In this scenario, 98% of the time spent was in Array.Resize.

Callers that know before hand that they are going to write a large CBOR document currently don't have an easy way to give a hint that the internal buffer should be larger than the default.

Conversely, callers that know they are going to create small CBOR documents may give a smaller-than-the-default hint to avoid over-allocating the internal buffer.

This is similar to what we did for AsnWriter except now we are doing it for CborWriter.

API Proposal

namespace System.Formats.Cbor;

public partial class CborWriter {
    public CborWriter(int initialCapacity, CborConformanceMode conformanceMode = CborConformanceMode.Strict, bool convertIndefiniteLengthEncodings = false, bool allowMultipleRootLevelValues = false);
}

API Usage

CborWriter writer = new CborWriter(initialCapacity: 10000000);
// write 1000000 bytes of stuff to the writer.

Alternative Designs

No response

Risks

No response

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Formats.Cbor

Milestone: -

@eiriktsarpalis
Copy link
Member

Sounds reasonable to me.

@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Sep 13, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 13, 2023
@eiriktsarpalis eiriktsarpalis 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 13, 2023
@bartonjs
Copy link
Member

bartonjs commented Sep 21, 2023

Video

  • We changed from an initial required parameter to a trailing defaulted parameter (and consequently will update the existing constructor to no longer have defaults, and now be never-browsable
namespace System.Formats.Cbor;

public partial class CborWriter {
    [EditorBrowsable(Never)]
    public CborWriter(
       CborConformanceMode conformanceMode,
       bool convertIndefiniteLengthEncodings,
       bool allowMultipleRootLevelValues);

    public CborWriter(
       CborConformanceMode conformanceMode = CborConformanceMode.Strict,
       bool convertIndefiniteLengthEncodings = false,
       bool allowMultipleRootLevelValues = false,
       int initialCapacity = -1);
}

@bartonjs bartonjs 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 Sep 21, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 23, 2023
@vcsjones vcsjones self-assigned this Sep 23, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 24, 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.Formats.Cbor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants