Skip to content

Improve Binary Xml (XmlDictionaryWriter) performance #71478

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

Merged
StephenMolloy merged 30 commits into from
Apr 1, 2023
Merged

Improve Binary Xml (XmlDictionaryWriter) performance #71478

Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c327b28
Improve perf of XmlBinaryWriter using Unsafe
Daniel-Svensson Jun 28, 2022
d57b7f2
Use ReverseEndianness to simplify writes
Daniel-Svensson Jun 28, 2022
c214302
Use span in a few places
Daniel-Svensson Jun 28, 2022
0e736f6
Add back old comparison, but only write float as int if it saves space
Daniel-Svensson Jun 29, 2022
d1e850b
Use span in array writing to reduce bounds checks
Daniel-Svensson Jun 29, 2022
d80b67f
Extract all unsafe code to shared method
Daniel-Svensson Jun 30, 2022
d24de76
Fix nodetype type
Daniel-Svensson Jun 30, 2022
c98689b
Avoid pinning and unsafe when writing arrays
Daniel-Svensson Jul 1, 2022
cf64f69
Merge remote-tracking branch 'upstream/main' into dev
Daniel-Svensson Jul 3, 2022
450e4d8
add back CheckArray
Daniel-Svensson Jul 3, 2022
149b6d9
Fix Assert condition
Daniel-Svensson Jul 4, 2022
2907e0a
Apply suggestions from code review
Daniel-Svensson Jul 8, 2022
86496bb
Fix review comments
Daniel-Svensson Jul 8, 2022
ed26c86
Merge remote-tracking branch 'upstream/main' into dev
Daniel-Svensson Jul 8, 2022
657e9b9
use new span ctor
Daniel-Svensson Jul 8, 2022
0a7a7e5
add tests
Daniel-Svensson Jul 25, 2022
963315d
Add tests for very long arrays
Daniel-Svensson Jul 25, 2022
c2e11d0
Fixx overload resolution for WriteText method
Daniel-Svensson Jul 26, 2022
e832548
Just copy guid arrays on LittleEndian platforms
Daniel-Svensson Aug 3, 2022
1d1818e
Merge remote-tracking branch 'upstream/main' into dev
Daniel-Svensson Aug 14, 2022
571a628
Add testversion of XmlBinaryNodeType with hardcoded values to use sam…
Daniel-Svensson Aug 14, 2022
5488341
call corrreect version of getbuffer
Daniel-Svensson Aug 15, 2022
c983179
Merge branch 'main' into dev
Daniel-Svensson Aug 16, 2022
ce69dce
Update src/libraries/System.Runtime.Serialization.Xml/tests/Reflectio…
Daniel-Svensson Aug 16, 2022
9281073
fix merge conflict
Daniel-Svensson Aug 16, 2022
798286a
Fix merge conflicts.
StephenMolloy Mar 16, 2023
ab3e0cc
Restored Big-Endian functionality. Ugly though.
StephenMolloy Mar 16, 2023
477cf8b
Cleaned up arrays.
StephenMolloy Mar 17, 2023
f51e07d
Merge branch 'main' into dev
StephenMolloy Mar 25, 2023
1fb4ba0
Remove leftover using in tests.
StephenMolloy Mar 31, 2023
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
Next Next commit
Improve perf of XmlBinaryWriter using Unsafe
  • Loading branch information
@Daniel-Svensson
Daniel-Svensson committed Jun 29, 2022
commit c327b28d17b571ef4a82b93681644ac8a7c0c4ee
205 changes: 105 additions & 100 deletions src/libraries/System.Private.DataContractSerialization/src/System/Xml/XmlBinaryWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
using System.Runtime.Serialization;
using System.Globalization;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

namespace System.Xml
{
Expand Down Expand Up @@ -72,6 +74,12 @@ private void WriteTextNode(XmlBinaryNodeType nodeType)
_textNodeOffset = this.BufferOffset - 1;
}

private ref byte GetBufferRef(int size)
Copy link
Member

@jkotas jkotas Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This code was using safe C# with proper boundschecks. You are switching this to use unsafe code without boundschecks. It is increasing security risk profile of this code.

Can this stay with safe C# and Spans? You should be still able to get nice performance gains by doing that.

Copy link
Contributor Author

@Daniel-Svensson Daniel-Svensson Jun 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes there was quite much unsafe code and points present in the file so I assumed it was ok to be unsafe for the best performance sine GetBuffer does the bounds check.

I've refactored the code so that all new "unsafe" code is now only present in 2 methods to make it much easier to reason about and updated the benchmark with timings for both Unsafe and Span.

The span version does .AsSpan(offset, size) on the buffer and then uses MemoryMarshal.Write<T> which I just foud out about.. I hope that method is considered safe enough and works well with "unaligned writes".

Please have a look and let me know based on the new code and the benchmark results which version you want.
I can push the updated span version instead if that version is the prefered one.

Copy link
Contributor Author

@Daniel-Svensson Daniel-Svensson Aug 15, 2022

Choose a reason for hiding this comment

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

Note: A side effect of moving to the span version is that it will probably be necessary to keep separate implementationa for writing 8bit (and maybe 16bit) nodes in order to not regress performance for them.

Some benchmarks would be required to determine the performance numbers for smaller elements first

=> ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(GetBuffer(size, out int offset)), offset);

private ref byte GetTextNodeBufferRef(int size)
=> ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(GetTextNodeBuffer(size, out int offset)), offset);

private byte[] GetTextNodeBuffer(int size, out int offset)
{
if (_inAttribute)
Expand All @@ -84,56 +92,74 @@ private byte[] GetTextNodeBuffer(int size, out int offset)
private void WriteTextNodeWithLength(XmlBinaryNodeType nodeType, int length)
{
DiagnosticUtility.DebugAssert(nodeType == XmlBinaryNodeType.Chars8Text || nodeType == XmlBinaryNodeType.Bytes8Text || nodeType == XmlBinaryNodeType.UnicodeChars8Text, "");
int offset;
byte[] buffer = GetTextNodeBuffer(5, out offset);
if (length < 256)
{
buffer[offset + 0] = (byte)nodeType;
buffer[offset + 1] = (byte)length;
Advance(2);
WriteTextNodeWithInt8(nodeType, unchecked((byte)length));
}
else if (length < 65536)
{
buffer[offset + 0] = (byte)(nodeType + 1 * 2); // WithEndElements interleave
buffer[offset + 1] = unchecked((byte)length);
length >>= 8;
buffer[offset + 2] = (byte)length;
Advance(3);
WriteTextNodeWithInt16(nodeType + /* WithEndElements interleave */ 1 * 2, unchecked((short)length));
}
else
{
buffer[offset + 0] = (byte)(nodeType + 2 * 2); // WithEndElements interleave
buffer[offset + 1] = (byte)length;
length >>= 8;
buffer[offset + 2] = (byte)length;
length >>= 8;
buffer[offset + 3] = (byte)length;
length >>= 8;
buffer[offset + 4] = (byte)length;
Advance(5);
WriteTextNodeWithInt32(nodeType + /* WithEndElements interleave */ 2 * 2, length);
}
}

private void WriteTextNodeWithInt8(XmlBinaryNodeType nodeType, byte value)
{
ref byte bytePtr = ref GetTextNodeBufferRef(2);
bytePtr = (byte)nodeType;
Unsafe.Add(ref bytePtr, 1) = value;
Advance(2);
}

private void WriteTextNodeWithInt16(XmlBinaryNodeType nodeType, short value)
{
ref byte bytePtr = ref GetTextNodeBufferRef(3);
bytePtr = (byte)nodeType;
Unsafe.Add(ref bytePtr, 1) = (byte)(value);
Unsafe.Add(ref bytePtr, 2) = (byte)(value >> 8);
Advance(3);
}

private void WriteTextNodeWithInt32(XmlBinaryNodeType nodeType, int value)
{
ref byte bytePtr = ref GetTextNodeBufferRef(5);
bytePtr = (byte)nodeType;
if (BitConverter.IsLittleEndian)
{
Unsafe.WriteUnaligned(ref Unsafe.Add(ref bytePtr, 1), value);
}
else
{
Unsafe.Add(ref bytePtr, 1) = (byte)(value);
Unsafe.Add(ref bytePtr, 2) = (byte)(value >> 8);
Unsafe.Add(ref bytePtr, 3) = (byte)(value >> 16);
Unsafe.Add(ref bytePtr, 4) = (byte)(value >> 24);
}
Advance(5);
}

private void WriteTextNodeWithInt64(XmlBinaryNodeType nodeType, long value)
{
int offset;
byte[] buffer = GetTextNodeBuffer(9, out offset);
buffer[offset + 0] = (byte)nodeType;
buffer[offset + 1] = (byte)value;
value >>= 8;
buffer[offset + 2] = (byte)value;
value >>= 8;
buffer[offset + 3] = (byte)value;
value >>= 8;
buffer[offset + 4] = (byte)value;
value >>= 8;
buffer[offset + 5] = (byte)value;
value >>= 8;
buffer[offset + 6] = (byte)value;
value >>= 8;
buffer[offset + 7] = (byte)value;
value >>= 8;
buffer[offset + 8] = (byte)value;
ref byte bytePtr = ref GetTextNodeBufferRef(9);
bytePtr = (byte)nodeType;
if (BitConverter.IsLittleEndian)
{
Unsafe.WriteUnaligned(ref Unsafe.Add(ref bytePtr, 1), value);
}
else
{
Unsafe.Add(ref bytePtr, 1) = (byte)(value);
Unsafe.Add(ref bytePtr, 2) = (byte)((uint)value >> 8);
Unsafe.Add(ref bytePtr, 3) = (byte)((uint)value >> 16);
Unsafe.Add(ref bytePtr, 4) = (byte)((uint)value >> 24);
Unsafe.Add(ref bytePtr, 5) = (byte)(value >> 32);
Unsafe.Add(ref bytePtr, 6) = (byte)(value >> 40);
Unsafe.Add(ref bytePtr, 7) = (byte)(value >> 48);
Unsafe.Add(ref bytePtr, 8) = (byte)(value >> 56);
}
Advance(9);
}

Expand Down Expand Up @@ -458,54 +484,37 @@ public override void WriteBoolText(bool value)

public override void WriteInt32Text(int value)
{
if (value >= -128 && value < 128)
if (value == (sbyte)value)
{
if (value == 0)
{
WriteTextNode(XmlBinaryNodeType.ZeroText);
}
else if (value == 1)
if ((byte)value <= 1)
{
WriteTextNode(XmlBinaryNodeType.OneText);
if ((byte)value < 1)
{
WriteTextNode(XmlBinaryNodeType.ZeroText);
}
else
{
WriteTextNode(XmlBinaryNodeType.OneText);
}
}
else
{
int offset;
byte[] buffer = GetTextNodeBuffer(2, out offset);
buffer[offset + 0] = (byte)XmlBinaryNodeType.Int8Text;
buffer[offset + 1] = (byte)value;
Advance(2);
WriteTextNodeWithInt8(XmlBinaryNodeType.Int8Text, (byte)value);
}
}
else if (value >= -32768 && value < 32768)
else if (value == (short)value)
{
int offset;
byte[] buffer = GetTextNodeBuffer(3, out offset);
buffer[offset + 0] = (byte)XmlBinaryNodeType.Int16Text;
buffer[offset + 1] = (byte)value;
value >>= 8;
buffer[offset + 2] = (byte)value;
Advance(3);
WriteTextNodeWithInt16(XmlBinaryNodeType.Int16Text, (short)value);
}
else
{
int offset;
byte[] buffer = GetTextNodeBuffer(5, out offset);
buffer[offset + 0] = (byte)XmlBinaryNodeType.Int32Text;
buffer[offset + 1] = (byte)value;
value >>= 8;
buffer[offset + 2] = (byte)value;
value >>= 8;
buffer[offset + 3] = (byte)value;
value >>= 8;
buffer[offset + 4] = (byte)value;
Advance(5);
WriteTextNodeWithInt32(XmlBinaryNodeType.Int32Text, (short)value);
}
}

public override void WriteInt64Text(long value)
{
if (value >= int.MinValue && value <= int.MaxValue)
if (value == (int)(value))
{
WriteInt32Text((int)value);
}
Expand All @@ -529,23 +538,22 @@ public override void WriteUInt64Text(ulong value)

private void WriteInt64(long value)
{
int offset;
byte[] buffer = GetBuffer(8, out offset);
buffer[offset + 0] = (byte)value;
value >>= 8;
buffer[offset + 1] = (byte)value;
value >>= 8;
buffer[offset + 2] = (byte)value;
value >>= 8;
buffer[offset + 3] = (byte)value;
value >>= 8;
buffer[offset + 4] = (byte)value;
value >>= 8;
buffer[offset + 5] = (byte)value;
value >>= 8;
buffer[offset + 6] = (byte)value;
value >>= 8;
buffer[offset + 7] = (byte)value;
ref byte bytePtr = ref GetBufferRef(8);
if (BitConverter.IsLittleEndian)
{
Unsafe.WriteUnaligned(ref bytePtr, value);
}
else
{
Unsafe.Add(ref bytePtr, 0) = (byte)(value);
Unsafe.Add(ref bytePtr, 1) = (byte)((uint)value >> 8);
Unsafe.Add(ref bytePtr, 2) = (byte)((uint)value >> 16);
Unsafe.Add(ref bytePtr, 3) = (byte)((uint)value >> 24);
Unsafe.Add(ref bytePtr, 4) = (byte)(value >> 32);
Unsafe.Add(ref bytePtr, 5) = (byte)(value >> 40);
Unsafe.Add(ref bytePtr, 6) = (byte)(value >> 48);
Unsafe.Add(ref bytePtr, 7) = (byte)(value >> 56);
}
Advance(8);
}

Expand Down Expand Up @@ -792,17 +800,12 @@ public unsafe override void WriteDoubleText(double d)
}
}

public unsafe override void WriteDecimalText(decimal d)
public override void WriteDecimalText(decimal d)
{
int offset;
byte[] buffer = GetTextNodeBuffer(1 + sizeof(decimal), out offset);
byte* bytes = (byte*)&d;
buffer[offset++] = (byte)XmlBinaryNodeType.DecimalText;
for (int i = 0; i < sizeof(decimal); i++)
{
buffer[offset++] = bytes[i];
}
Advance(1 + sizeof(decimal));
ref byte bytePtr = ref GetTextNodeBufferRef(1 + Unsafe.SizeOf<decimal>());
bytePtr = (byte)XmlBinaryNodeType.DecimalText;
Unsafe.WriteUnaligned(ref Unsafe.Add(ref bytePtr, 1), d);
Advance(1 + Unsafe.SizeOf<decimal>());
}

public override void WriteDateTimeText(DateTime dt)
Expand Down Expand Up @@ -848,10 +851,12 @@ public override void WriteUniqueIdText(UniqueId value)

public override void WriteGuidText(Guid guid)
{
int offset;
byte[] buffer = GetTextNodeBuffer(17, out offset);
buffer[offset] = (byte)XmlBinaryNodeType.GuidText;
Buffer.BlockCopy(guid.ToByteArray(), 0, buffer, offset + 1, 16);
var span = GetTextNodeBuffer(17, out int offset).AsSpan(offset, 17);

span[0] = (byte)XmlBinaryNodeType.GuidText;
bool ok = guid.TryWriteBytes(span.Slice(1));
DiagnosticUtility.DebugAssert(ok, "");

Advance(17);
}

Expand Down