Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Add char[] buffer to XmlRawWriter. by TrayanZapryanov · Pull Request #75411 · dotnet/runtime · GitHub
Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add char[] buffer to XmlRawWriter. by TrayanZapryanov · Pull Request #75411 · dotnet/runtime · GitHub
Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add char[] buffer to XmlRawWriter. by TrayanZapryanov · Pull Request #75411 · dotnet/runtime · GitHub
Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Add char[] buffer to XmlRawWriter. by TrayanZapryanov · Pull Request #75411 · dotnet/runtime · GitHub
Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Add char[] buffer to XmlRawWriter. by TrayanZapryanov · Pull Request #75411 · dotnet/runtime · GitHub
Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); })(); Add char[] buffer to XmlRawWriter. by TrayanZapryanov · Pull Request #75411 · dotnet/runtime · GitHub
Skip to content

Add char[] buffer to XmlRawWriter. - #75411

Closed
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters
Closed

Add char[] buffer to XmlRawWriter.#75411
TrayanZapryanov wants to merge 11 commits into
dotnet:mainfrom
TrayanZapryanov:xml_use_char_buffer_in_xmlwriters

Conversation

@TrayanZapryanov

@TrayanZapryanovTrayanZapryanov commented Sep 11, 2022

Copy link
Copy Markdown
Contributor

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it. Then using WriteRaw(char[]) method instead of creating temporary string and WriteRaw(string) method.
Also exposing XmlConverter.TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.
Sample optimization

Expose TryFormat methods in XmlConvert and use them in XmlRawWriter.
Expose TryFormat in XsdDateTime and XsdDuration.
@ghostghost added area-System.Xml community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-xml
See info in area-owners.md if you want to be subscribed.

Issue Details

This PR suggest using small char[] buffer in XmlRawWriter and use it for formatting primitive types in it and then using WriteRaw(char[]) method instead of creating temporary string and using WriteRaw(string).
Also exposing TryFormat methods for already exposed ToString(xxx) primitive types, allowing writes to Span destination.
Last change is to expose TryFormat in XsdDateTime and XsdDuration.

This PR will open possibilities in XmlSerializers to use typed methods of XmlWriter instead of always fallback to WriteString one.

Author:TrayanZapryanov
Assignees:-
Labels:

area-System.Xml

Milestone:-

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
_primitivesBuffer = new char[_primitivesBuffer.Length * 2];
}

WriteChars(_primitivesBuffer, 0, charsWritten);

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I saw that there is difference between

publicoverrideunsafevoidWriteChars(char[]buffer,intindex,intcount)

and

publicoverrideunsafevoidWriteRaw(char[]buffer,intindex,intcount)

Which one should be used ?

{
// For compatibility with custom writers, XmlWriter writes DateTimeOffset as DateTime.
// Our internal writers should use the DateTimeOffset-String conversion from XmlConvert.
WriteString(XmlConvert.ToString(value));

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This comment seems not relevant as even now it is using

publicstaticstringToString(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Maybe it is connected with base code ?

publicvirtualvoidWriteValue(DateTimeOffsetvalue)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@krwq Do you know if somebody is changing this Mode ?

internalstaticclassXmlCustomFormatter
{
privatestaticDateTimeSerializationSection.DateTimeSerializationModes_mode;
privatestaticDateTimeSerializationSection.DateTimeSerializationModeMode
{
get
{
if(s_mode==DateTimeSerializationSection.DateTimeSerializationMode.Default)
{
s_mode=DateTimeSerializationSection.DateTimeSerializationMode.Roundtrip;
}
returns_mode;
}
}

I saw similar code in XmlSerializers and the only way to change is with Reflection and directly manipulating fiedl.

Comment threadsrc/libraries/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.cs Outdated
Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
WriteWithBuffer((char)value, XmlConvert.TryFormat);
break;
default:
//Guid is not supported in XmlUntypedConverter.Untyped and throws exception

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This limitation is really strange, but we need to adapt XmlValueConverter to allow Guid in it's methods.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

return CreateException(index == 0 ? SR.Xml_BadStartNameChar : SR.Xml_BadNameChar, XmlException.BuildCharExceptionArgs(name, index), exceptionType, 0, index + 1);
}

public static bool TryFormat(bool value, Span<char> destination, out int charsWritten)

@PaulusParssinenPaulusParssinenSep 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this one can do same optimization as in #64782 but with correct casing.

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlRawWriter.cs Outdated
}

// Override in order to handle Xml simple typed values and to pass resolver for QName values
public override void WriteValue(object value)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Whole logic for writing with buffer is duplicated in XmlTextWriter and XmlRawWriter.
One suggestion is to create "public" class( this is needed as XmlTextWriter is public) and encapsulate logic in it.
The other is to have some static helper class which works by ref with char[].

Which one do you prefer ?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Duplication logic reduced, but the question still stays open.

@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

@krwq, @eiriktsarpalis Is it possible to receive some initial feedback on these changes and to the following ones(changes in serializators to use typed methods instead of converting to string first) ?
Do you see any value in this idea or you expect some problem later that will stop it?
Should I continue with following PRs to change one by one serializers(Reflection, Primitive, IlGen....) based on this or to wait until this one is merged?
I am quite new in proposing changes and will need some guidance if possible.

ArgumentNullException.ThrowIfNull(value);

WriteString(XmlUntypedConverter.Untyped.ToString(value, _resolver));
Type sourceType = value.GetType();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems to be adding a lot of branching where it previously didn't exist. Could this be done differently? I'm not super familiar with this code, but one thought might be to expose a TryFormat method to XmlUntypedConverter.Untyped.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The code in XmlUntypedConverter.ToString(object) is already similar:
```cs
public override string ToString(object value, IXmlNamespaceResolver? nsResolver)
{
ArgumentNullException.ThrowIfNull(value);

 Type sourceType = value.GetType();
if (sourceType == BooleanType) return XmlConvert.ToString((bool)value);
if (sourceType == ByteType) return XmlConvert.ToString((byte)value);
if (sourceType == ByteArrayType) return Base64BinaryToString((byte[])value);
if (sourceType == DateTimeType) return DateTimeToString((DateTime)value);
if (sourceType == DateTimeOffsetType) return DateTimeOffsetToString((DateTimeOffset)value);
if (sourceType == DecimalType) return XmlConvert.ToString((decimal)value);
if (sourceType == DoubleType) return XmlConvert.ToString((double)value);
if (sourceType == Int16Type) return XmlConvert.ToString((short)value);
if (sourceType == Int32Type) return XmlConvert.ToString((int)value);
if (sourceType == Int64Type) return XmlConvert.ToString((long)value);
if (sourceType == SByteType) return XmlConvert.ToString((sbyte)value);
if (sourceType == SingleType) return XmlConvert.ToString((float)value);
if (sourceType == StringType) return ((string)value);
if (sourceType == TimeSpanType) return DurationToString((TimeSpan)value);
if (sourceType == UInt16Type) return XmlConvert.ToString((ushort)value);
if (sourceType == UInt32Type) return XmlConvert.ToString((uint)value);
if (sourceType == UInt64Type) return XmlConvert.ToString((ulong)value);
if (IsDerivedFrom(sourceType, UriType)) return AnyUriToString((Uri)value);
if (sourceType == XmlAtomicValueType) return ((string)((XmlAtomicValue)value).ValueAs(StringType, nsResolver));
if (IsDerivedFrom(sourceType, XmlQualifiedNameType)) return QNameToString((XmlQualifiedName)value, nsResolver);
return (string)ChangeTypeWildcardDestination(value, StringType, nsResolver);
}

Comment threadsrc/libraries/System.Private.Xml/src/System/Xml/Core/XmlTextWriter.cs Outdated
@TrayanZapryanov

Copy link
Copy Markdown
ContributorAuthor

Closing this in favor of 76436

@TrayanZapryanov
TrayanZapryanov deleted the xml_use_char_buffer_in_xmlwriters branch September 30, 2022 10:42
@ghostghost locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Xmlcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@TrayanZapryanov@krwq@eiriktsarpalis@PaulusParssinen@Trayan-Zapryanov