Skip to content

Improve performance of buffered string write #5985

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.

Sign up for GitHub

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

Closed
bbowyersmyth wants to merge 2 commits into from
Closed

Improve performance of buffered string write #5985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
90dfc86
Improve performance of buffered string write
bbowyersmyth Jan 2, 2025
18f91bd
Nested
bbowyersmyth Jan 2, 2025
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
2 changes: 1 addition & 1 deletion src/Npgsql/Internal/Converters/FullTextSearch/TsQueryConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ async Task WriteCore(NpgsqlTsQuery node)
writer.WriteByte(lexemeNode.IsPrefixSearch ? (byte)1 : (byte)0);

if (async)
await writer.WriteCharsAsync(lexemeNode.Text.AsMemory(), encoding, cancellationToken).ConfigureAwait(false);
await writer.WriteCharsAsync(lexemeNode.Text.AsMemory(), encoding, cancellationToken: cancellationToken).ConfigureAwait(false);
else
writer.WriteChars(lexemeNode.Text.AsMemory().Span, encoding);

Expand Down
2 changes: 1 addition & 1 deletion src/Npgsql/Internal/Converters/FullTextSearch/TsVectorConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ async ValueTask Write(bool async, PgWriter writer, NpgsqlTsVector value, Cancell
foreach (var lexeme in value)
{
if (async)
await writer.WriteCharsAsync(lexeme.Text.AsMemory(), encoding, cancellationToken).ConfigureAwait(false);
await writer.WriteCharsAsync(lexeme.Text.AsMemory(), encoding, cancellationToken: cancellationToken).ConfigureAwait(false);
else
writer.WriteChars(lexeme.Text.AsMemory().Span, encoding);

Expand Down
4 changes: 2 additions & 2 deletions src/Npgsql/Internal/Converters/HstoreConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ async ValueTask Write(bool async, PgWriter writer, T value, CancellationToken ca
var length = size.Value;
writer.WriteInt32(length);
if (async)
await writer.WriteCharsAsync(kv.Key.AsMemory(), encoding, cancellationToken).ConfigureAwait(false);
await writer.WriteCharsAsync(kv.Key.AsMemory(), encoding, cancellationToken: cancellationToken).ConfigureAwait(false);
else
writer.WriteChars(kv.Key.AsSpan(), encoding);

Expand All @@ -138,7 +138,7 @@ async ValueTask Write(bool async, PgWriter writer, T value, CancellationToken ca
if (valueLength is not -1)
{
if (async)
await writer.WriteCharsAsync(kv.Value.AsMemory(), encoding, cancellationToken).ConfigureAwait(false);
await writer.WriteCharsAsync(kv.Value.AsMemory(), encoding, cancellationToken: cancellationToken).ConfigureAwait(false);
else
writer.WriteChars(kv.Value.AsSpan(), encoding);
}
Expand Down
20 changes: 10 additions & 10 deletions src/Npgsql/Internal/Converters/Primitive/TextConverters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
// ReSharper disable once CheckNamespace
namespace Npgsql.Internal.Converters;

abstract class StringBasedTextConverter<T>(Encoding encoding) : PgStreamingConverter<T>
abstract class StringBasedTextConverter<T>(Encoding encoding, bool nested = true) : PgStreamingConverter<T>
{
public override T Read(PgReader reader)
=> Read(async: false, reader, encoding).GetAwaiter().GetResult();
Expand All @@ -23,10 +23,10 @@ public override Size GetSize(SizeContext context, T value, ref object? writeStat
=> TextConverter.GetSize(ref context, ConvertTo(value), encoding);

public override void Write(PgWriter writer, T value)
=> writer.WriteChars(ConvertTo(value).Span, encoding);
=> writer.WriteChars(ConvertTo(value).Span, encoding, !nested ? writer.Current.Size.Value : null);

public override ValueTask WriteAsync(PgWriter writer, T value, CancellationToken cancellationToken = default)
=> writer.WriteCharsAsync(ConvertTo(value), encoding, cancellationToken);
=> writer.WriteCharsAsync(ConvertTo(value), encoding, !nested ? writer.Current.Size.Value : null, cancellationToken : cancellationToken);

public override bool CanConvert(DataFormat format, out BufferRequirements bufferRequirements)
{
Expand All @@ -49,13 +49,13 @@ async ValueTask<T> ReadAsync(PgReader reader, Encoding encoding, CancellationTok
}
}

sealed class ReadOnlyMemoryTextConverter(Encoding encoding) : StringBasedTextConverter<ReadOnlyMemory<char>>(encoding)
sealed class ReadOnlyMemoryTextConverter(Encoding encoding, bool nested = true) : StringBasedTextConverter<ReadOnlyMemory<char>>(encoding, nested)
{
protected override ReadOnlyMemory<char> ConvertTo(ReadOnlyMemory<char> value) => value;
protected override ReadOnlyMemory<char> ConvertFrom(string value) => value.AsMemory();
}

sealed class StringTextConverter(Encoding encoding) : StringBasedTextConverter<string>(encoding)
sealed class StringTextConverter(Encoding encoding, bool nested = true) : StringBasedTextConverter<string>(encoding, nested)
{
protected override ReadOnlyMemory<char> ConvertTo(string value) => value.AsMemory();
protected override string ConvertFrom(string value) => value;
Expand All @@ -75,7 +75,7 @@ public override void Write(PgWriter writer, T value)
=> writer.WriteChars(ConvertTo(value).AsSpan(), encoding);

public override ValueTask WriteAsync(PgWriter writer, T value, CancellationToken cancellationToken = default)
=> writer.WriteCharsAsync(ConvertTo(value), encoding, cancellationToken);
=> writer.WriteCharsAsync(ConvertTo(value), encoding, cancellationToken: cancellationToken);

public override bool CanConvert(DataFormat format, out BufferRequirements bufferRequirements)
{
Expand Down Expand Up @@ -122,7 +122,7 @@ protected override char[] ConvertFrom(ArraySegment<char> value)
}
}

sealed class CharTextConverter(Encoding encoding) : PgBufferedConverter<char>
sealed class CharTextConverter(Encoding encoding, bool nested = true) : PgBufferedConverter<char>
{
readonly Size _oneCharMaxByteCount = Size.CreateUpperBound(encoding.GetMaxByteCount(1));

Expand All @@ -149,14 +149,14 @@ protected override char ReadCore(PgReader reader)

public override Size GetSize(SizeContext context, char value, ref object? writeState)
{
Span<char> spanValue = [value];
ReadOnlySpan<char> spanValue = [value];
return encoding.GetByteCount(spanValue);
}

protected override void WriteCore(PgWriter writer, char value)
{
Span<char> spanValue = [value];
writer.WriteChars(spanValue, encoding);
ReadOnlySpan<char> spanValue = [value];
writer.WriteChars(spanValue, encoding, !nested ? writer.Current.Size.Value : null);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Npgsql/Internal/PgReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -744,10 +744,10 @@ public bool ShouldBuffer(Size bufferRequirement)
=> ShouldBuffer(GetBufferRequirementByteCount(bufferRequirement));
public bool ShouldBuffer(int byteCount)
{
return _buffer.ReadBytesLeft < byteCount && ShouldBufferSlow();
return _buffer.ReadBytesLeft < byteCount && ShouldBufferSlow(byteCount);

[MethodImpl(MethodImplOptions.NoInlining)]
bool ShouldBufferSlow()
bool ShouldBufferSlow(int byteCount)
{
if (byteCount > _buffer.Size)
ThrowHelper.ThrowArgumentOutOfRangeException(nameof(byteCount),
Expand Down
12 changes: 8 additions & 4 deletions src/Npgsql/Internal/PgWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,13 +271,15 @@ public void WriteDouble(double value)
Advance(sizeof(double));
}

public void WriteChars(ReadOnlySpan<char> data, Encoding encoding)
public void WriteChars(ReadOnlySpan<char> data, Encoding encoding, int? encodedByteLength = null)
Copy link
Copy Markdown
Member

@NinoFloris NinoFloris Feb 4, 2025

Choose a reason for hiding this comment

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

I don't think we should make these error-prone methods public. Instead we can apply the optimization to the internal converters. I'm fine having these signatures accessible internally.

{
// If we have more chars than bytes remaining we can immediately go to the slow path.
if (data.Length <= Remaining)
{
// If not, it's worth a shot to see if we can convert in one go.
var encodedLength = encoding.GetByteCount(data);
Debug.Assert(encodedByteLength is null || encodedByteLength == encoding.GetByteCount(data));
var encodedLength = encodedByteLength ?? encoding.GetByteCount(data);

if (!ShouldFlush(encodedLength))
{
var count = encoding.GetBytes(data, Span);
Expand Down Expand Up @@ -305,14 +307,16 @@ void Core(ReadOnlySpan<char> data, Encoding encoding)
}
}

public ValueTask WriteCharsAsync(ReadOnlyMemory<char> data, Encoding encoding, CancellationToken cancellationToken = default)
public ValueTask WriteCharsAsync(ReadOnlyMemory<char> data, Encoding encoding, int? encodedByteLength = null, CancellationToken cancellationToken = default)
{
var dataSpan = data.Span;
// If we have more chars than bytes remaining we can immediately go to the slow path.
if (data.Length <= Remaining)
{
// If not, it's worth a shot to see if we can convert in one go.
var encodedLength = encoding.GetByteCount(dataSpan);
Debug.Assert(encodedByteLength is null || encodedByteLength == encoding.GetByteCount(dataSpan));
var encodedLength = encodedByteLength ?? encoding.GetByteCount(dataSpan);

if (!ShouldFlush(encodedLength))
{
var count = encoding.GetBytes(dataSpan, Span);
Expand Down
8 changes: 4 additions & 4 deletions src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,9 @@ static TypeInfoMappingCollection AddMappings(TypeInfoMappingCollection mappings)
// Text
// Update PgSerializerOptions.IsWellKnownTextType(Type) after any changes to this list.
mappings.AddType<string>(DataTypeNames.Text,
static (options, mapping, _) => mapping.CreateInfo(options, new StringTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text), isDefault: true);
static (options, mapping, _) => mapping.CreateInfo(options, new StringTextConverter(options.TextEncoding, nested: false), preferredFormat: DataFormat.Text), isDefault: true);
Copy link
Copy Markdown
Member

@NinoFloris NinoFloris Feb 4, 2025

Choose a reason for hiding this comment

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

Nestedness is not a mapping invariant. For instance this converter will be composed over by the array converter (via AddArrayType) and it would do the wrong thing. You'd have to look at the writer (we could maybe have some state there) to know whether the converter is called in a top-level context.

Copy link
Copy Markdown
Contributor Author

@bbowyersmyth bbowyersmyth Feb 8, 2025

Choose a reason for hiding this comment

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

Ok I'll take this PR back to draft for a while. I'll have to get a better understanding of the mapping to know what the next steps are.

mappings.AddStructType<char>(DataTypeNames.Text,
static (options, mapping, _) => mapping.CreateInfo(options, new CharTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text));
static (options, mapping, _) => mapping.CreateInfo(options, new CharTextConverter(options.TextEncoding, nested: false), preferredFormat: DataFormat.Text));
// Uses the bytea converters, as neither type has a header.
mappings.AddType<byte[]>(DataTypeNames.Text,
static (options, mapping, _) => mapping.CreateInfo(options, new ArrayByteaConverter()),
Expand All @@ -103,9 +103,9 @@ static TypeInfoMappingCollection AddMappings(TypeInfoMappingCollection mappings)
DataTypeNames.Xml, DataTypeNames.Name, DataTypeNames.RefCursor })
{
mappings.AddType<string>(dataTypeName,
static (options, mapping, _) => mapping.CreateInfo(options, new StringTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text), isDefault: true);
static (options, mapping, _) => mapping.CreateInfo(options, new StringTextConverter(options.TextEncoding, nested: false), preferredFormat: DataFormat.Text), isDefault: true);
mappings.AddStructType<char>(dataTypeName,
static (options, mapping, _) => mapping.CreateInfo(options, new CharTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text));
static (options, mapping, _) => mapping.CreateInfo(options, new CharTextConverter(options.TextEncoding, nested: false), preferredFormat: DataFormat.Text));
// Uses the bytea converters, as neither type has a header.
mappings.AddType<byte[]>(dataTypeName,
static (options, mapping, _) => mapping.CreateInfo(options, new ArrayByteaConverter()),
Expand Down
4 changes: 2 additions & 2 deletions src/Npgsql/Internal/ResolverFactories/ExtraConversionsTypeInfoResolverFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ static TypeInfoMappingCollection AddInfos(TypeInfoMappingCollection mappings)
mappings.AddType<char[]>(DataTypeNames.Text,
static (options, mapping, _) => mapping.CreateInfo(options, new CharArrayTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text));
mappings.AddStructType<ReadOnlyMemory<char>>(DataTypeNames.Text,
static (options, mapping, _) => mapping.CreateInfo(options, new ReadOnlyMemoryTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text));
static (options, mapping, _) => mapping.CreateInfo(options, new ReadOnlyMemoryTextConverter(options.TextEncoding, nested: false), preferredFormat: DataFormat.Text));
mappings.AddStructType<ArraySegment<char>>(DataTypeNames.Text,
static (options, mapping, _) => mapping.CreateInfo(options, new CharArraySegmentTextConverter(options.TextEncoding), preferredFormat: DataFormat.Text));

Expand All @@ -121,7 +121,7 @@ static TypeInfoMappingCollection AddInfos(TypeInfoMappingCollection mappings)
static (options, mapping, _) => mapping.CreateInfo(options, new CharArrayTextConverter(options.TextEncoding),
preferredFormat: DataFormat.Text));
mappings.AddStructType<ReadOnlyMemory<char>>(dataTypeName,
static (options, mapping, _) => mapping.CreateInfo(options, new ReadOnlyMemoryTextConverter(options.TextEncoding),
static (options, mapping, _) => mapping.CreateInfo(options, new ReadOnlyMemoryTextConverter(options.TextEncoding, nested: false),
preferredFormat: DataFormat.Text));
mappings.AddStructType<ArraySegment<char>>(dataTypeName,
static (options, mapping, _) => mapping.CreateInfo(options, new CharArraySegmentTextConverter(options.TextEncoding),
Expand Down
8 changes: 4 additions & 4 deletions test/Npgsql.Benchmarks/TypeHandlers/Text.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
using BenchmarkDotNet.Attributes;
using System.Collections.Generic;
using System.Text;
using Npgsql.Internal;
using Npgsql.Internal.Converters;
using System.Collections.Generic;

namespace Npgsql.Benchmarks.TypeHandlers;

[Config(typeof(Config))]
public class Text() : TypeHandlerBenchmarks<string>(new StringTextConverter(Encoding.UTF8))
public class Text() : TypeHandlerBenchmarks<string>(new StringTextConverter(NpgsqlWriteBuffer.UTF8Encoding, nested: false))
{
protected override IEnumerable<string> ValuesOverride()
{
for (var i = 1; i <= 10000; i *= 10)
for (var i = 1; i <= 1000; i *= 10)
yield return new string('x', i);
}
}
2 changes: 1 addition & 1 deletion test/Npgsql.Tests/TypeMapperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ sealed class Resolver : IPgTypeInfoResolver
{
if (type == typeof(string) || dataTypeName?.UnqualifiedName == "citext")
if (options.DatabaseInfo.TryGetPostgresTypeByName("citext", out var pgType))
return new(options, new StringTextConverter(options.TextEncoding), options.ToCanonicalTypeId(pgType));
return new(options, new StringTextConverter(options.TextEncoding, nested: false), options.ToCanonicalTypeId(pgType));

return null;
}
Expand Down