-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Re-design Utf8JsonWriter as a regular class rather than a ref struct. #36961
Changes from 13 commits
a19d268
b522e0c
341d937
9caf27c
38a3834
61f51c4
1406858
449d935
ec4d03f
5a060d1
fc5c82a
b44b6b8
d6213e3
50e832c
f94ca69
6aa5efc
5af4802
695c9df
4cecd6a
4d7a90e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,187 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Diagnostics; | ||
|
|
||
| namespace System.Buffers | ||
| { | ||
| /// <summary> | ||
| /// Represents a heap-based, array-backed output sink into which <typeparam name="T"/> data can be written. | ||
| /// </summary> | ||
| #if USE_ABW_INTERNALLY | ||
| internal | ||
| #else | ||
| public | ||
| #endif | ||
| sealed class ArrayBufferWriter<T> : IBufferWriter<T> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to match the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd rather not artificially constrain it, unless there's good reason to. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The scenario is a bit edge case IMO - when would you pass a writeable buffer of objects? There's an indirection in memory (element->object) and "ownership" now becomes a bit more ambiguous - i.e. what if the objects implement IDispose -- who's going to call that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's no different than storing objects into an array; when do you pass arrays of things, like strings? You also can have the exact same ownership question with structs, which might themselves have resources that have lifetime, or wrap any number of reference types. Limiting it is just an artificial restriction. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that limiting isn't a good thing if we don't need to. Since Memory has lifetime semantics (don't hold onto it after your method returns etc) it is not the same as an array. Consider a buffer type that is only for data (or primitives like char or byte - not objects) and in that case there could be some memory clear and copy-like efficiencies regarding clearing after use and shuffling unprocessed elements to the beginning when making room for more data. Originally I was thinking we may be doing some of these operations here which would make it unsafe for objects... |
||
| { | ||
| private T[] _buffer; | ||
| private int _index; | ||
|
|
||
| private const int MinimumBufferSize = 256; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't actually MinimumBufferSize, at least not how it's currently used. A better name for the current usage would be DefaultInitialBufferSize, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I was thinking the same thing. The only concern is that I use this to define the minimum size GetMemory/GetSpan can return as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It isn't the minimum, though. It's just the value that you pretend sizeHint was if it's 0. (GetSpan(1) is allowed to return a span of Length 1) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. I meant minimum in the "default" case, but you brought up a useful clarification. |
||
|
|
||
| /// <summary> | ||
| /// Creates an instance of an <see cref="ArrayBufferWriter{T}"/>, in which data can be written to, | ||
| /// with the default initial capacity. | ||
| /// </summary> | ||
| public ArrayBufferWriter() | ||
| { | ||
| _buffer = new T[MinimumBufferSize]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why wouldn't we start with |
||
| _index = 0; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates an instance of an <see cref="ArrayBufferWriter{T}"/>, in which data can be written to, | ||
| /// with an initial capacity specified. | ||
| /// </summary> | ||
| /// <param name="initialCapacity">The minimum capacity with which to initialize the underlying buffer.</param> | ||
| /// <exception cref="ArgumentException"> | ||
| /// Thrown when <paramref name="initialCapacity"/> is not positive (i.e. less than or equal to 0). | ||
| /// </exception> | ||
| public ArrayBufferWriter(int initialCapacity) | ||
| { | ||
| if (initialCapacity <= 0) | ||
| throw new ArgumentException(nameof(initialCapacity)); | ||
|
|
||
| _buffer = new T[initialCapacity]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want to bound |
||
| _index = 0; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns the data written to the underlying buffer so far, as a <see cref="ReadOnlyMemory{T}"/>. | ||
| /// </summary> | ||
| public ReadOnlyMemory<T> WrittenMemory => _buffer.AsMemory(0, _index); | ||
|
|
||
| /// <summary> | ||
| /// Returns the data written to the underlying buffer so far, as a <see cref="ReadOnlySpan{T}"/>. | ||
| /// </summary> | ||
| public ReadOnlySpan<T> WrittenSpan => _buffer.AsSpan(0, _index); | ||
ahsonkhan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| /// <summary> | ||
| /// Returns the amount of data written to the underlying buffer so far. | ||
| /// </summary> | ||
| public int WrittenCount => _index; | ||
|
|
||
| /// <summary> | ||
| /// Returns the total amount of space within the underlying buffer. | ||
| /// </summary> | ||
| public int Capacity => _buffer.Length; | ||
|
|
||
| /// <summary> | ||
| /// Returns the amount of space available that can still be written into without forcing the underlying buffer to grow. | ||
| /// </summary> | ||
| public int FreeCapacity => _buffer.Length - _index; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need this? Someone can just do Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The serializer uses this but you are right it is primarily a convenience thing. Do you think its exposing too much or do you just think it is unnecessary and not worth the benefit it provides? Regarding the naming/API changes like this, how do you feel about discussing it all up in a follow up review (post preview 5)? I prefer not to make these changes in PR (in isolation). WrittenCount fits nicer with WrittenMemory/Span (especially when passed in as "offset/count" to stream.write), imo, but I am not super attached to the name. |
||
|
|
||
| /// <summary> | ||
| /// Clears the data written to the underlying buffer. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// You must clear the <see cref="ArrayBufferWriter{T}"/> before trying to re-use it. | ||
| /// </remarks> | ||
| public void Clear() | ||
| { | ||
| Debug.Assert(_buffer.Length >= _index); | ||
| _buffer.AsSpan(0, _index).Clear(); | ||
| _index = 0; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Notifies <see cref="IBufferWriter{T}"/> that <paramref name="count"/> amount of data was written to the output <see cref="Span{T}"/>/<see cref="Memory{T}"/> | ||
| /// </summary> | ||
| /// <exception cref="ArgumentException"> | ||
| /// Thrown when <paramref name="count"/> is negative. | ||
| /// </exception> | ||
| /// <exception cref="InvalidOperationException"> | ||
| /// Thrown when attempting to advance past the end of the underlying buffer. | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// You must request a new buffer after calling Advance to continue writing more data and cannot write to a previously acquired buffer. | ||
| /// </remarks> | ||
| public void Advance(int count) | ||
| { | ||
| if (count < 0) | ||
| throw new ArgumentException(nameof(count)); | ||
|
|
||
| if (_index > _buffer.Length - count) | ||
| ThrowInvalidOperationException(_buffer.Length); | ||
|
|
||
| _index += count; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns a <see cref="Memory{T}"/> to write to that is at least the requested length (specified by <paramref name="sizeHint"/>). | ||
| /// If no <paramref name="sizeHint"/> is provided (or it's equal to <code>0</code>), some non-empty buffer is returned. | ||
| /// </summary> | ||
| /// <exception cref="ArgumentException"> | ||
| /// Thrown when <paramref name="sizeHint"/> is negative. | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// This will never return an empty <see cref="Memory{T}"/> but it can throw | ||
| /// if the requested buffer size is not available. | ||
| /// </remarks> | ||
| /// <remarks> | ||
| /// There is no guarantee that successive calls will return the same buffer or the same-sized buffer. | ||
| /// </remarks> | ||
| /// <remarks> | ||
| /// You must request a new buffer after calling Advance to continue writing more data and cannot write to a previously acquired buffer. | ||
| /// </remarks> | ||
| public Memory<T> GetMemory(int sizeHint = 0) | ||
| { | ||
| CheckAndResizeBuffer(sizeHint); | ||
| Debug.Assert(_buffer.Length >= _index); | ||
ahsonkhan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return _buffer.AsMemory(_index); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns a <see cref="Span{T}"/> to write to that is at least the requested length (specified by <paramref name="sizeHint"/>). | ||
| /// If no <paramref name="sizeHint"/> is provided (or it's equal to <code>0</code>), some non-empty buffer is returned. | ||
| /// </summary> | ||
| /// <exception cref="ArgumentException"> | ||
| /// Thrown when <paramref name="sizeHint"/> is negative. | ||
| /// </exception> | ||
| /// <remarks> | ||
| /// This will never return an empty <see cref="Span{T}"/> but it can throw | ||
ahsonkhan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| /// if the requested buffer size is not available. | ||
| /// </remarks> | ||
| /// <remarks> | ||
| /// There is no guarantee that successive calls will return the same buffer or the same-sized buffer. | ||
| /// </remarks> | ||
| /// <remarks> | ||
| /// You must request a new buffer after calling Advance to continue writing more data and cannot write to a previously acquired buffer. | ||
| /// </remarks> | ||
| public Span<T> GetSpan(int sizeHint = 0) | ||
| { | ||
| CheckAndResizeBuffer(sizeHint); | ||
| Debug.Assert(_buffer.Length >= _index); | ||
| return _buffer.AsSpan(_index); | ||
| } | ||
|
|
||
| private void CheckAndResizeBuffer(int sizeHint) | ||
| { | ||
| if (sizeHint < 0) | ||
| throw new ArgumentException(nameof(sizeHint)); | ||
|
|
||
| if (sizeHint == 0) | ||
| { | ||
| sizeHint = MinimumBufferSize; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any feedback/concerns on this growth strategy? Any call to GetSpan/GetMemory will return at least 256 bytes, if sizeHint is not specified. This has consequences when used with an ArrayBufferWriter ctor that has initialCapacity. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was contemplating this when looking earlier. I'd say that sizeHint of 0 should imply sizeHint of 1 (simply "not empty" but avoids growing if it can) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had considered this, but in practice, most workloads would suffer from this since they can't make forward progress with 1 byte. It effectively makes calling GetSpan without an argument useless. I am not sure returning 1 is better. If there are no strong opinions here, I would let scenario-driven user feedback guide the change, and leave it as is for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I dunno, once the regrow happens it'll double in size, so you'll end up with one call between 1 and 256, then the "oh, I need more than that..." would get a bigger buffer.
Also seems valid, but if the caller really doesn't have any notion of a hint then it seems like any size is acceptable. Otherwise passing in min/max/accurate seems like the best bet. |
||
| } | ||
|
|
||
| if (sizeHint > FreeCapacity) | ||
| { | ||
| int growBy = Math.Max(sizeHint, _buffer.Length); | ||
|
|
||
| int newSize = checked(_buffer.Length + growBy); | ||
|
|
||
| Array.Resize(ref _buffer, newSize); | ||
| } | ||
|
|
||
| Debug.Assert(FreeCapacity > 0 && FreeCapacity >= sizeHint); | ||
| } | ||
|
|
||
| private static void ThrowInvalidOperationException(int capacity) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Seems like this should be named |
||
| { | ||
| throw new InvalidOperationException(SR.Format(SR.BufferWriterAdvancedTooFar, capacity)); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this pattern acceptable to use? If not, I am open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do this in other places as well, but I think it would be better to invert the condition, such that the type is only public if you explicitly set a compilation constant... otherwise, if a project includes this file, by default it's going to be exposing a new type publicly.