Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve allocations in ReflectionXmlSerializationWriter when serializing primitive types #84474

Conversation

TrayanZapryanov
Copy link
Contributor

Follow up of #76436
More primitives are serialized into char buffer and then written into writer, instead of serialized to string.

Next step is to use similar methods/approach in XmlSerializationWriterILGen.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 7, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@TrayanZapryanov TrayanZapryanov marked this pull request as draft April 7, 2023 13:00
@TrayanZapryanov
Copy link
Contributor Author

@mconnew, @StephenMolloy
Do you see any value in these changes?
They introduce fast path serializing primitive types as elements/attributes. Which is supposed to succeed, but if not - then the old logic stays.
If I can read code correctly XmlSerializationWriterILGen is to produce code similar to the ReflectionXmlSerializationWriter, just skipping reflection part.
So the code that we embed into reflectionwriter should be easily portable int ILGen one.
Once similar changes are in XmlSerializationWriterILGen code, then I see the biggest benefit of reducing allocations of all primitive types...

@@ -39,7 +39,7 @@ public abstract class XmlSerializationWriter : XmlSerializationGeneratedCode
private bool _escapeName = true;

//char buffer for serializing primitive values
private readonly char[] _primitivesBuffer = new char[64];
protected readonly char[] primitivesBuffer = new char[64];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is optional - can be private and then we just have another buffer here.

@@ -585,6 +585,7 @@ protected partial class Fixup
public abstract partial class XmlSerializationWriter : System.Xml.Serialization.XmlSerializationGeneratedCode
{
protected XmlSerializationWriter() { }
protected readonly char[] primitivesBuffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels a bit weird. Are there other places in ref assemblies where protected fields are exposed?

@StephenMolloy
Copy link
Member

@mconnew, @StephenMolloy Do you see any value in these changes? They introduce fast path serializing primitive types as elements/attributes. Which is supposed to succeed, but if not - then the old logic stays. If I can read code correctly XmlSerializationWriterILGen is to produce code similar to the ReflectionXmlSerializationWriter, just skipping reflection part. So the code that we embed into reflectionwriter should be easily portable int ILGen one. Once similar changes are in XmlSerializationWriterILGen code, then I see the biggest benefit of reducing allocations of all primitive types...

It will be a little while before we get a chance to review these changes. But assuming they are like the last PR, I would anticipate the improvements are welcome. A couple of notes:

  1. Use private fields for the buffer. We don't want to find ourselves in a situation where pre-generated serializers start relying on the protected field that doesn't exist on all potential runtimes.
  2. Our plan is to try to migrate away from the ILGen serializer implementation. The code is tricky and messy in every way. I'd stay away from it. We are unlikely to accept any PR's in that area.

@TrayanZapryanov
Copy link
Contributor Author

@StephenMolloy Fair enough.
I also think that IlGen is not for everyone and will close this PR if this is the direction.
Can you share a bit your plans for xml serialization, so I can check if there's something that I can help?

@StephenMolloy
Copy link
Member

@TrayanZapryanov - Did I miss something? This draft is for the reflection-based serializer, which is the direction we do want to move. My second point above was responding to

the code that we embed into reflectionwriter should be easily portable int ILGen one. Once similar changes are in XmlSerializationWriterILGen code...

I didn't want you or anybody else to go down that ILGen road since it likely doesn't play a big part in the future.

@TrayanZapryanov
Copy link
Contributor Author

@StephenMolloy Sorry I somehow misread your statement and decided that you don't like PRs for Xml serialization and have plans to replace it with something else.. Maybe something connected with source generators...
I will check again this draft and try to tune it, so don't worry for the review. Do it when you have time.

@ghost ghost closed this May 21, 2023
@ghost
Copy link

ghost commented May 21, 2023

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 21, 2023
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Serialization community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants