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

Use the array pool in Json writer #1420

Merged
merged 2 commits into from
Mar 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
17 changes: 14 additions & 3 deletions src/Microsoft.OData.Core/Buffers/BufferUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,23 @@ internal static class BufferUtils
/// <returns>The initialized buffer</returns>
public static char[] InitializeBufferIfRequired(char[] buffer)
{
if (buffer == null)
return InitializeBufferIfRequired(null, buffer);
Copy link

Choose a reason for hiding this comment

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

Why do we need this method? can't we do something like this?
if (buffer == null) {
return RentFromBuffer(null, buffer);
}
return buffer;

Copy link
Member Author

@xuzhg xuzhg Mar 14, 2019

Choose a reason for hiding this comment

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

That's avoiding a duplicated code. At later, if we change something, we only change the second method, don't need to change all methods. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with that pattern.


In reply to: 265697058 [](ancestors = 265697058)

}

/// <summary>
/// Checks if the buffer is not initialized and if initialized returns the same buffer or creates a new one.
/// </summary>
/// <param name="bufferPool">The character pool.</param>
/// <param name="buffer">The buffer to verify</param>
/// <returns>The initialized buffer.</returns>
public static char[] InitializeBufferIfRequired(ICharArrayPool bufferPool, char[] buffer)
{
if (buffer != null)
{
return new char[BufferUtils.BufferLength];
return buffer;
}

return buffer;
return RentFromBuffer(bufferPool, BufferLength);
}

/// <summary>
Expand Down
16 changes: 8 additions & 8 deletions src/Microsoft.OData.Core/Json/JsonValueUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ internal static void WriteValue(TextWriter writer, sbyte value)
/// <param name="value">String value to be written.</param>
/// <param name="stringEscapeOption">The string escape option.</param>
/// <param name="buffer">Char buffer to use for streaming data</param>
internal static void WriteValue(TextWriter writer, string value, ODataStringEscapeOption stringEscapeOption, ref char[] buffer)
internal static void WriteValue(TextWriter writer, string value, ODataStringEscapeOption stringEscapeOption, ref char[] buffer, ICharArrayPool arrayPool = null)
{
Debug.Assert(writer != null, "writer != null");

Expand All @@ -307,7 +307,7 @@ internal static void WriteValue(TextWriter writer, string value, ODataStringEsca
}
else
{
JsonValueUtils.WriteEscapedJsonString(writer, value, stringEscapeOption, ref buffer);
JsonValueUtils.WriteEscapedJsonString(writer, value, stringEscapeOption, ref buffer, arrayPool);
}
}

Expand All @@ -317,7 +317,7 @@ internal static void WriteValue(TextWriter writer, string value, ODataStringEsca
/// <param name="writer">The text writer to write the output to.</param>
/// <param name="value">Byte array to be written.</param>
/// <param name="buffer">Char buffer to use for streaming data.</param>
internal static void WriteValue(TextWriter writer, byte[] value, ref char[] buffer)
internal static void WriteValue(TextWriter writer, byte[] value, ref char[] buffer, ICharArrayPool arrayPool = null)
{
Debug.Assert(writer != null, "writer != null");

Expand All @@ -328,7 +328,7 @@ internal static void WriteValue(TextWriter writer, byte[] value, ref char[] buff
else
{
writer.Write(JsonConstants.QuoteCharacter);
WriteBinaryString(writer, value, ref buffer);
WriteBinaryString(writer, value, ref buffer, arrayPool);
writer.Write(JsonConstants.QuoteCharacter);
}
}
Expand All @@ -339,12 +339,12 @@ internal static void WriteValue(TextWriter writer, byte[] value, ref char[] buff
/// <param name="writer">The text writer to write the output to.</param>
/// <param name="value">Byte array to be written.</param>
/// <param name="buffer">Char buffer to use for streaming data.</param>
internal static void WriteBinaryString(TextWriter writer, byte[] value, ref char[] buffer)
internal static void WriteBinaryString(TextWriter writer, byte[] value, ref char[] buffer, ICharArrayPool arrayPool)
{
Debug.Assert(writer != null, "writer != null");
Debug.Assert(value != null, "The value must not be null.");

buffer = BufferUtils.InitializeBufferIfRequired(buffer);
buffer = BufferUtils.InitializeBufferIfRequired(arrayPool, buffer);
Debug.Assert(buffer != null);

int bufferLength = buffer.Length;
Expand Down Expand Up @@ -373,7 +373,7 @@ internal static void WriteBinaryString(TextWriter writer, byte[] value, ref char
/// <param name="stringEscapeOption">The string escape option.</param>
/// <param name="buffer">Char buffer to use for streaming data</param>
internal static void WriteEscapedJsonString(TextWriter writer, string inputString,
ODataStringEscapeOption stringEscapeOption, ref char[] buffer)
ODataStringEscapeOption stringEscapeOption, ref char[] buffer, ICharArrayPool bufferPool = null)
{
Debug.Assert(writer != null, "writer != null");
Debug.Assert(inputString != null, "The string value must not be null.");
Expand All @@ -390,7 +390,7 @@ internal static void WriteEscapedJsonString(TextWriter writer, string inputStrin
int inputStringLength = inputString.Length;

Debug.Assert(firstIndex < inputStringLength, "First index of the special character should be within the string");
buffer = BufferUtils.InitializeBufferIfRequired(buffer);
buffer = BufferUtils.InitializeBufferIfRequired(bufferPool, buffer);
int bufferLength = buffer.Length;
int bufferIndex = 0;
int currentIndex = 0;
Expand Down
23 changes: 21 additions & 2 deletions src/Microsoft.OData.Core/Json/JsonWriter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ namespace Microsoft.OData.Json
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.IO;
using Microsoft.OData.Buffers;
using Microsoft.OData.Edm;
#endregion Namespaces

/// <summary>
/// Writer for the JSON format. http://www.json.org
/// </summary>
[SuppressMessage("Microsoft.Design", "CA1001:TypesThatOwnDisposableFieldsShouldBeDisposable", Justification = "This class does not own the underlying stream/writer and thus should never dispose it.")]
internal sealed class JsonWriter : IJsonWriter
internal sealed class JsonWriter : IJsonWriter, IDisposable
{
/// <summary>
/// Writer to write text into.
Expand Down Expand Up @@ -93,6 +94,11 @@ internal enum ScopeType
Padding = 2,
}

/// <summary>
/// Get/sets the character buffer pool.
/// </summary>
public ICharArrayPool ArrayPool { get; set; }

/// <summary>
/// Start the padding function scope.
/// </summary>
Expand Down Expand Up @@ -374,7 +380,7 @@ public void WriteValue(string value)
public void WriteValue(byte[] value)
{
this.WriteValueSeparator();
JsonValueUtils.WriteValue(this.writer, value, ref this.buffer);
JsonValueUtils.WriteValue(this.writer, value, ref this.buffer, ArrayPool);
}

/// <summary>
Expand All @@ -395,6 +401,19 @@ public void Flush()
this.writer.Flush();
Copy link
Member

@mikepizzo mikepizzo Mar 19, 2019

Choose a reason for hiding this comment

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

Should we return to pool on Flush or Dispose? #Closed

Copy link
Member Author

@xuzhg xuzhg Mar 19, 2019

Choose a reason for hiding this comment

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

@mikepizzo I changed to use the "Dispose". #Closed

}

/// <summary>
/// Dispose the writer
/// </summary>
public void Dispose()
{
if (this.ArrayPool != null && this.buffer != null)
{
BufferUtils.ReturnToBuffer(this.ArrayPool, this.buffer);
this.ArrayPool = null;
this.buffer = null;
}
}

/// <summary>
/// Writes a separator of a value if it's needed for the next value to be written.
/// </summary>
Expand Down
30 changes: 23 additions & 7 deletions src/Microsoft.OData.Core/JsonLight/ODataJsonLightOutputContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace Microsoft.OData.JsonLight
using System.IO;
#if PORTABLELIB
using System.Threading.Tasks;
using Microsoft.OData.Buffers;
#endif
using Microsoft.OData.Edm;
using Microsoft.OData.Json;
Expand Down Expand Up @@ -86,7 +87,7 @@ public ODataJsonLightOutputContext(

// COMPAT 2: JSON indentation - WCFDS indents only partially, it inserts newlines but doesn't actually insert spaces for indentation
// in here we allow the user to specify if true indentation should be used or if the limited functionality is enough.
this.jsonWriter = CreateJsonWriter(this.Container, this.textWriter, messageInfo.MediaType.HasIeee754CompatibleSetToTrue());
this.jsonWriter = CreateJsonWriter(this.Container, this.textWriter, messageInfo.MediaType.HasIeee754CompatibleSetToTrue(), messageWriterSettings);
}
catch (Exception e)
{
Expand Down Expand Up @@ -121,7 +122,7 @@ internal ODataJsonLightOutputContext(
Debug.Assert(messageWriterSettings != null, "messageWriterSettings != null");

this.textWriter = textWriter;
this.jsonWriter = CreateJsonWriter(messageInfo.Container, textWriter, true /*isIeee754Compatible*/);
this.jsonWriter = CreateJsonWriter(messageInfo.Container, textWriter, true /*isIeee754Compatible*/, messageWriterSettings);
this.metadataLevel = new JsonMinimalMetadataLevel();
this.propertyCacheHandler = new PropertyCacheHandler();
}
Expand Down Expand Up @@ -769,6 +770,12 @@ protected override void Dispose(bool disposing)
// The TextWriter.Flush (Which is in fact StreamWriter.Flush) will call the underlying Stream.Flush.
this.jsonWriter.Flush();

JsonWriter writer = this.jsonWriter as JsonWriter;
if (writer != null)
{
writer.Dispose();
}

// In the async case the underlying stream is the async buffered stream, so we have to flush that explicitly.
if (this.asynchronousOutputStream != null)
{
Expand All @@ -791,16 +798,25 @@ protected override void Dispose(bool disposing)
base.Dispose(disposing);
}

private static IJsonWriter CreateJsonWriter(IServiceProvider container, TextWriter textWriter, bool isIeee754Compatible)
private static IJsonWriter CreateJsonWriter(IServiceProvider container, TextWriter textWriter, bool isIeee754Compatible, ODataMessageWriterSettings writerSettings)
{
IJsonWriter jsonWriter;
if (container == null)
{
return new JsonWriter(textWriter, isIeee754Compatible);
jsonWriter = new JsonWriter(textWriter, isIeee754Compatible);
}
else
{
var jsonWriterFactory = container.GetRequiredService<IJsonWriterFactory>();
jsonWriter = jsonWriterFactory.CreateJsonWriter(textWriter, isIeee754Compatible);
Debug.Assert(jsonWriter != null, "jsonWriter != null");
}

var jsonWriterFactory = container.GetRequiredService<IJsonWriterFactory>();
var jsonWriter = jsonWriterFactory.CreateJsonWriter(textWriter, isIeee754Compatible);
Debug.Assert(jsonWriter != null, "jsonWriter != null");
JsonWriter writer = jsonWriter as JsonWriter;
if (writer != null && writerSettings.ArrayPool != null)
{
writer.ArrayPool = writerSettings.ArrayPool;
}

return jsonWriter;
}
Expand Down
3 changes: 1 addition & 2 deletions src/Microsoft.OData.Core/ODataMessageReaderSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ public ODataMessageReaderSettings(ODataVersion odataVersion)
public ODataVersion? Version { get; set; }

/// <summary>
/// Get/sets the character buffer pool. This pool is used for non-container scenario.
/// If the service container has an array pool, that pool will be used.
/// Get/sets the character buffer pool.
/// </summary>
public ICharArrayPool ArrayPool { get; set; }

Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.OData.Core/ODataMessageWriterSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace Microsoft.OData
{
#region Namespaces
using System;
using Microsoft.OData.Buffers;
using Microsoft.OData.UriParser;
#endregion Namespaces

Expand Down Expand Up @@ -133,6 +134,11 @@ public Uri BaseUri
/// the provided function name and parenthesis for JSONP. Otherwise this value is ignored.</remarks>
public string JsonPCallback { get; set; }

/// <summary>
/// Get/sets the character buffer pool.
/// </summary>
public ICharArrayPool ArrayPool { get; set; }

/// <summary>
/// Quotas to use for limiting resource consumption when writing an OData message.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5332,6 +5332,7 @@ public sealed class Microsoft.OData.ODataMessageWriter : IDisposable {
public sealed class Microsoft.OData.ODataMessageWriterSettings {
public ODataMessageWriterSettings ()

Microsoft.OData.Buffers.ICharArrayPool ArrayPool { public get; public set; }
System.Uri BaseUri { public get; public set; }
bool EnableCharactersCheck { public get; public set; }
bool EnableMessageStreamDisposal { public get; public set; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ namespace Microsoft.Test.Taupo.OData.Writer.Tests.Json
using Microsoft.Test.Taupo.OData.Common;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Microsoft.OData.Json;
using Microsoft.OData.Buffers;

/// <summary>
/// Tests for the ODataJsonConvert class
Expand Down Expand Up @@ -285,7 +286,7 @@ public static void WriteObjectValue(TextWriter writer, object value)
public static void WriteValue(TextWriter writer, TimeSpan value) { ReflectionUtils.InvokeMethod(classType, "WriteValue", writer, value); }
public static void WriteValue(TextWriter writer, byte value) { ReflectionUtils.InvokeMethod(classType, "WriteValue", writer, value); }
public static void WriteValue(TextWriter writer, sbyte value) { ReflectionUtils.InvokeMethod(classType, "WriteValue", writer, value); }
public static void WriteValue(TextWriter writer, string value) { char[] buffer = null; ReflectionUtils.InvokeMethod(classType, "WriteValue", new Type[] { typeof(TextWriter), typeof(string), typeof(ODataStringEscapeOption), typeof(char[]).MakeByRefType() }, writer, value, ODataStringEscapeOption.EscapeNonAscii, buffer); }
public static void WriteValue(TextWriter writer, string value) { char[] buffer = null; ReflectionUtils.InvokeMethod(classType, "WriteValue", new Type[] { typeof(TextWriter), typeof(string), typeof(ODataStringEscapeOption), typeof(char[]).MakeByRefType(), typeof(ICharArrayPool) }, writer, value, ODataStringEscapeOption.EscapeNonAscii, buffer, null); }
}
#endif
}
Expand Down