More Code Review Updates

* Add more testing for ReadOnlySpan parsing
  * Update `AssertReadFromParseContext` to also run assertions for a
`ParseContext` initialized from a `ReadOnlySpan` of the given input.
* Remove passing `ReadOnlySpan` parameters by ref since they are not
altered and the type isn't large enough to benefit from performance
improvements for passing by `ref` or `in`
* Update `ParseContext` `Initialize` overloads for
`ReadOnlySpan<byte>`, going down to two (since nothing was calling the
overload that changed the recursion limit). The first now calls the
second to set the `buffer` and `state` on the context, and has
documentation to note that it is used to create a `ParserInternalState`
with default settings.
pull/8473/head
Jensaarai 4 years ago
parent 2f0e269725
commit 1252f3e147
  1. 9
      csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs
  2. 3
      csharp/src/Google.Protobuf/CodedInputStream.cs
  3. 6
      csharp/src/Google.Protobuf/MessageExtensions.cs
  4. 4
      csharp/src/Google.Protobuf/MessageParser.cs
  5. 38
      csharp/src/Google.Protobuf/ParseContext.cs

@ -161,12 +161,21 @@ namespace Google.Protobuf
private static void AssertReadFromParseContext(ReadOnlySequence<byte> input, ParseContextAssertAction assertAction, bool assertIsAtEnd) private static void AssertReadFromParseContext(ReadOnlySequence<byte> input, ParseContextAssertAction assertAction, bool assertIsAtEnd)
{ {
// Check as ReadOnlySequence<byte>
ParseContext.Initialize(input, out ParseContext parseCtx); ParseContext.Initialize(input, out ParseContext parseCtx);
assertAction(ref parseCtx); assertAction(ref parseCtx);
if (assertIsAtEnd) if (assertIsAtEnd)
{ {
Assert.IsTrue(SegmentedBufferHelper.IsAtEnd(ref parseCtx.buffer, ref parseCtx.state)); Assert.IsTrue(SegmentedBufferHelper.IsAtEnd(ref parseCtx.buffer, ref parseCtx.state));
} }
// Check as ReadOnlySpan<byte>
ParseContext.Initialize(input.ToArray().AsSpan(), out ParseContext spanParseContext);
assertAction(ref spanParseContext);
if (assertIsAtEnd)
{
Assert.IsTrue(SegmentedBufferHelper.IsAtEnd(ref spanParseContext.buffer, ref spanParseContext.state));
}
} }
[Test] [Test]

@ -435,8 +435,7 @@ namespace Google.Protobuf
// we will need to switch back again to CodedInputStream-based parsing (which involves copying and storing the state) to be able to // we will need to switch back again to CodedInputStream-based parsing (which involves copying and storing the state) to be able to
// invoke the legacy MergeFrom(CodedInputStream) method. // invoke the legacy MergeFrom(CodedInputStream) method.
// For now, this inefficiency is fine, considering this is only a backward-compatibility scenario (and regenerating the code fixes it). // For now, this inefficiency is fine, considering this is only a backward-compatibility scenario (and regenerating the code fixes it).
var span = new ReadOnlySpan<byte>(buffer); ParseContext.Initialize(buffer.AsSpan(), ref state, out ParseContext ctx);
ParseContext.Initialize(ref span, ref state, out ParseContext ctx);
try try
{ {
ParsingPrimitivesMessages.ReadMessage(ref ctx, builder); ParsingPrimitivesMessages.ReadMessage(ref ctx, builder);

@ -86,7 +86,7 @@ namespace Google.Protobuf
/// <param name="span">Span containing the data to merge, which must be protobuf-encoded binary data.</param> /// <param name="span">Span containing the data to merge, which must be protobuf-encoded binary data.</param>
[SecuritySafeCritical] [SecuritySafeCritical]
public static void MergeFrom(this IMessage message, ReadOnlySpan<byte> span) => public static void MergeFrom(this IMessage message, ReadOnlySpan<byte> span) =>
MergeFrom(message, ref span, false, null); MergeFrom(message, span, false, null);
/// <summary> /// <summary>
/// Merges length-delimited data from the given stream into an existing message. /// Merges length-delimited data from the given stream into an existing message.
@ -304,9 +304,9 @@ namespace Google.Protobuf
} }
[SecuritySafeCritical] [SecuritySafeCritical]
internal static void MergeFrom(this IMessage message, ref ReadOnlySpan<byte> data, bool discardUnknownFields, ExtensionRegistry registry) internal static void MergeFrom(this IMessage message, ReadOnlySpan<byte> data, bool discardUnknownFields, ExtensionRegistry registry)
{ {
ParseContext.Initialize(ref data, out ParseContext ctx); ParseContext.Initialize(data, out ParseContext ctx);
ctx.DiscardUnknownFields = discardUnknownFields; ctx.DiscardUnknownFields = discardUnknownFields;
ctx.ExtensionRegistry = registry; ctx.ExtensionRegistry = registry;
ParsingPrimitivesMessages.ReadRawMessage(ref ctx, message); ParsingPrimitivesMessages.ReadRawMessage(ref ctx, message);

@ -137,7 +137,7 @@ namespace Google.Protobuf
public IMessage ParseFrom(ReadOnlySpan<byte> data) public IMessage ParseFrom(ReadOnlySpan<byte> data)
{ {
IMessage message = factory(); IMessage message = factory();
message.MergeFrom(ref data, DiscardUnknownFields, Extensions); message.MergeFrom(data, DiscardUnknownFields, Extensions);
return message; return message;
} }
@ -337,7 +337,7 @@ namespace Google.Protobuf
public new T ParseFrom(ReadOnlySpan<byte> data) public new T ParseFrom(ReadOnlySpan<byte> data)
{ {
T message = factory(); T message = factory();
message.MergeFrom(ref data, DiscardUnknownFields, Extensions); message.MergeFrom(data, DiscardUnknownFields, Extensions);
return message; return message;
} }

@ -58,34 +58,30 @@ namespace Google.Protobuf
internal ReadOnlySpan<byte> buffer; internal ReadOnlySpan<byte> buffer;
internal ParserInternalState state; internal ParserInternalState state;
/// <summary>
/// Initialize a <see cref="ParseContext"/>, building all <see cref="ParserInternalState"/> from defaults and
/// the given <paramref name="buffer"/>.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)] [MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void Initialize(ref ReadOnlySpan<byte> buffer, ref ParserInternalState state, out ParseContext ctx) internal static void Initialize(ReadOnlySpan<byte> buffer, out ParseContext ctx)
{ {
ctx.buffer = buffer; ParserInternalState state = default;
ctx.state = state; state.sizeLimit = DefaultSizeLimit;
} state.recursionLimit = DefaultRecursionLimit;
state.currentLimit = int.MaxValue;
state.bufferSize = buffer.Length;
[MethodImpl(MethodImplOptions.AggressiveInlining)] Initialize(buffer, ref state, out ctx);
internal static void Initialize(ref ReadOnlySpan<byte> input, out ParseContext ctx)
{
Initialize(ref input, DefaultRecursionLimit, out ctx);
} }
/// <summary>
/// Initialize a <see cref="ParseContext"/> using existing <see cref="ParserInternalState"/>, e.g. from <see cref="CodedInputStream"/>.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)] [MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static void Initialize(ref ReadOnlySpan<byte> input, int recursionLimit, out ParseContext ctx) internal static void Initialize(ReadOnlySpan<byte> buffer, ref ParserInternalState state, out ParseContext ctx)
{ {
ctx.buffer = input; ctx.buffer = buffer;
ctx.state = default; ctx.state = state;
ctx.state.lastTag = 0;
ctx.state.recursionDepth = 0;
ctx.state.sizeLimit = DefaultSizeLimit;
ctx.state.recursionLimit = recursionLimit;
ctx.state.currentLimit = int.MaxValue;
ctx.state.bufferPos = 0;
ctx.state.bufferSize = input.Length;
ctx.state.DiscardUnknownFields = false;
ctx.state.ExtensionRegistry = null;
} }
/// <summary> /// <summary>

Loading…
Cancel
Save