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

Fix bugs causing separator to be omitted after ODataUtf8JsonWriter.WriteRawValue #2527

Merged
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
48535b6
Add tests to verify raw values
habbes Oct 13, 2022
9bcd90f
Add BitStack implementation
habbes Oct 13, 2022
ba37735
Fix ODataUtf8JsonWriter WriteRawValue bugs
habbes Oct 14, 2022
d1ae04c
Fix raw value handling in async ODataUtf8JsonWriter
habbes Oct 17, 2022
2822b80
Add raw values to benchmarks
habbes Oct 24, 2022
8d96c34
Write to buffer writer directly instead of writing to stream
habbes Nov 23, 2022
7a7178f
Transcode raw value directly to bufferwriter
habbes Nov 23, 2022
c1595eb
Use PooledByteBufferWriter to reduce allocs
habbes Nov 23, 2022
a7cc8cd
Flush if buffer written count approaches threshold
habbes Nov 23, 2022
857c744
Refactor async writer to use buffer writer and update benchmarks
habbes Nov 23, 2022
360501f
Remove obsolete PooledByteBufferWriter
habbes Nov 23, 2022
9817e0a
Test buffer flushing logic
habbes Nov 23, 2022
45c84e3
Remove obsolete comments
habbes Nov 24, 2022
0d60b3b
Add flushing edge case tests
habbes Nov 25, 2022
e09398a
Update src/Microsoft.OData.Core/Json/ODataUtf8JsonWriter.cs
habbes Nov 29, 2022
7cccade
Apply suggestions from code review
habbes Nov 29, 2022
65e98aa
Apply suggestions from code review
habbes Nov 29, 2022
c530efa
Update license file
habbes Nov 30, 2022
54a56d8
Refactor WriterRawValueCore and make minor fixes in tests
habbes Nov 30, 2022
fe5427e
Inline frequently called helper methods
habbes Nov 30, 2022
5bb5203
Address review comments
habbes Dec 2, 2022
63443bf
Rename writer to utf8JsonWriter
habbes Dec 2, 2022
64a8bcc
Rename method for clarity
habbes Dec 2, 2022
85068cc
Rename fields
habbes Dec 2, 2022
13286b5
Minor code fixes
habbes Dec 2, 2022
283f125
Minor fixes
habbes Dec 3, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion loadtests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ scenarios:
connections: 128
duration: 30
count: 50
includeRawValues: false
writer: 'ODataMessageWriter'
path: /customers/{{writer}}?count={{count}}
path: /customers/{{writer}}?count={{count}}&includeRawValues={{includeRawValues}}

profiles:
local:
Expand Down
179 changes: 179 additions & 0 deletions src/Microsoft.OData.Core/Json/BitStack.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
//---------------------------------------------------------------------
// <copyright file="ODataUtf8JsonWriterAsyncTests.cs" company="Microsoft">
habbes marked this conversation as resolved.
Show resolved Hide resolved
// Copyright (C) Microsoft Corporation. All rights reserved. See License.txt in the project root for license information.
// </copyright>
//---------------------------------------------------------------------

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;

namespace Microsoft.OData.Json
{
/// <summary>
/// An efficient stack optimized for storing data that has two possible kinds of value, represented
/// as true and false.
/// </summary>
/// <remarks>
/// This has been adapted from the .NET runtime's internal BitStack which is used by Utf8JsonWriter
/// https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/System/Text/Json/BitStack.cs
/// This has been slightly modified because the original Pop() method did not return the last value pushed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious. What does the original Pop() method return?

Copy link
Contributor

@ElizabethOkerio ElizabethOkerio Oct 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use the Utf8JsonWriter's WriteRawValue method in .net 6.0, do you still need to do this modification to the Pop method? That's if Utf8JsonWriter uses the BitStack internally to track when or how to use the separators.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to return the value of the item before the most recent item, or something like that.

/// </remarks>
internal struct BitStack
{
// We are using a ulong to represent our nested state, so we can only
// go 64 levels deep without having to allocate.
private const int AllocationFreeMaxDepth = sizeof(ulong) * 8;

private const int DefaultInitialArraySize = 2;

/// When the size of the stack exceeds 64, the following int array will be lazily allocated
/// to store additional values. Each int is a segment of the stack holding
/// up to 32 values. The current position of the stack is tracked by <see cref="_currentDepth"/>.
private int[] _array;

// This ulong container represents a tiny stack to track the state during nested transitions.
// The first bit represents the state of the current depth (1 == object, 0 == array).
// Each subsequent bit is the parent / containing type (object or array). Since this
// reader does a linear scan, we only need to keep a single path as we go through the data.
// This is primarily used as an optimization to avoid having to allocate an object for
// depths up to 64 (which is the default max depth).
private ulong _allocationFreeContainer;

private int _currentDepth;

/// <summary>
/// The current depth or size of the stack.
/// </summary>
public int CurrentDepth => _currentDepth;

/// <summary>
/// Pushes the value represented by `true` to the stack.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void PushTrue()
{
if (_currentDepth < AllocationFreeMaxDepth)
{
_allocationFreeContainer = (_allocationFreeContainer << 1) | 1;
}
else
{
PushToArray(true);
}
_currentDepth++;
}

/// <summary>
/// Pushes the value represented by `false` to the stack.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void PushFalse()
{
if (_currentDepth < AllocationFreeMaxDepth)
{
_allocationFreeContainer <<= 1;
}
else
{
PushToArray(false);
}
_currentDepth++;
}

/// Allocate the bit array lazily only when it is absolutely necessary.
[MethodImpl(MethodImplOptions.NoInlining)]
private void PushToArray(bool value)
{
_array = _array == null ? new int[DefaultInitialArraySize] : _array;

int index = _currentDepth - AllocationFreeMaxDepth;

Debug.Assert(index >= 0, $"Set - Negative - index: {index}, arrayLength: {_array.Length}");

// Maximum possible array length if bitLength was int.MaxValue (i.e. 67_108_864)
Debug.Assert(_array.Length <= int.MaxValue / 32 + 1, $"index: {index}, arrayLength: {_array.Length}");

int elementIndex = Div32Rem(index, out int extraBits);

// Grow the array when setting a bit if it isn't big enough
// This way the caller doesn't have to check.
if (elementIndex >= _array.Length)
{
// This multiplication can overflow, so cast to uint first.
Debug.Assert(index >= 0 && index > (int)((uint)_array.Length * 32 - 1), $"Only grow when necessary - index: {index}, arrayLength: {_array.Length}");
DoubleArray(elementIndex);
}

Debug.Assert(elementIndex < _array.Length, $"Set - index: {index}, elementIndex: {elementIndex}, arrayLength: {_array.Length}, extraBits: {extraBits}");

int newValue = _array[elementIndex];
if (value)
{
newValue |= 1 << extraBits;
}
else
{
newValue &= ~(1 << extraBits);
}
_array[elementIndex] = newValue;
}

/// <summary>
/// Removes the value at the top of the stack and returns it.
/// </summary>
/// <returns>The value at the top of the stack.</returns>
/// <remarks>You should ensure the stack is not empty before calling this method.</remarks>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool Pop()
{
_currentDepth--;
bool inObject;
if (_currentDepth < AllocationFreeMaxDepth)
{
inObject = (_allocationFreeContainer & 1) != 0;
_allocationFreeContainer >>= 1;
}
else
{
inObject = PopFromArray();
}

return inObject;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private bool PopFromArray()
{
int index = _currentDepth - AllocationFreeMaxDepth;
Debug.Assert(_array != null);
Debug.Assert(index >= 0, $"Get - Negative - index: {index}, arrayLength: {_array.Length}");

int elementIndex = Div32Rem(index, out int extraBits);

Debug.Assert(elementIndex < _array.Length, $"Get - index: {index}, elementIndex: {elementIndex}, arrayLength: {_array.Length}, extraBits: {extraBits}");

return (_array[elementIndex] & (1 << extraBits)) != 0;
}

private void DoubleArray(int minSize)
{
Debug.Assert(_array != null);
Debug.Assert(_array.Length < int.MaxValue / 2, $"Array too large - arrayLength: {_array.Length}");
Debug.Assert(minSize >= 0 && minSize >= _array.Length);

int nextDouble = Math.Max(minSize + 1, _array.Length * 2);
Debug.Assert(nextDouble > minSize);

Array.Resize(ref _array, nextDouble);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private static int Div32Rem(int number, out int remainder)
{
uint quotient = (uint)number / 32;
remainder = number & (32 - 1); // equivalent to number % 32, since 32 is a power of 2
return (int)quotient;
}
}
}
Loading