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

Use the array pool in Json writer #1420

Merged
merged 2 commits into from
Mar 21, 2019
Merged

Conversation

xuzhg
Copy link
Member

@xuzhg xuzhg commented Mar 9, 2019

Issues

  • Use the array pool in Json writer

Description

  • Use the array pool in Json writer

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@xuzhg xuzhg force-pushed the JsonWriterBuffer branch from 6d5f92d to 2bcae35 Compare March 9, 2019 00:41
@xuzhg
Copy link
Member Author

xuzhg commented Mar 9, 2019

@ysanghi Would you please also take a look this PR? thanks.

@xuzhg xuzhg force-pushed the JsonWriterBuffer branch from 2bcae35 to c558eff Compare March 12, 2019 17:20
@xuzhg xuzhg added the Ready for review Use this label if a pull request is ready to be reviewed label Mar 13, 2019
@@ -23,12 +23,23 @@ internal static class BufferUtils
/// <returns>The initialized buffer</returns>
public static char[] InitializeBufferIfRequired(char[] buffer)
{
if (buffer == null)
return InitializeBufferIfRequired(null, buffer);
Copy link

Choose a reason for hiding this comment

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

Why do we need this method? can't we do something like this?
if (buffer == null) {
return RentFromBuffer(null, buffer);
}
return buffer;

Copy link
Member Author

@xuzhg xuzhg Mar 14, 2019

Choose a reason for hiding this comment

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

That's avoiding a duplicated code. At later, if we change something, we only change the second method, don't need to change all methods. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with that pattern.


In reply to: 265697058 [](ancestors = 265697058)

@@ -392,6 +398,7 @@ public void WriteRawValue(string rawValue)
/// </summary>
public void Flush()
{
BufferUtils.ReturnToBuffer(this.ArrayPool, this.buffer);
this.writer.Flush();
Copy link
Member

@mikepizzo mikepizzo Mar 19, 2019

Choose a reason for hiding this comment

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

Should we return to pool on Flush or Dispose? #Closed

Copy link
Member Author

@xuzhg xuzhg Mar 19, 2019

Choose a reason for hiding this comment

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

@mikepizzo I changed to use the "Dispose". #Closed

@xuzhg xuzhg force-pushed the JsonWriterBuffer branch from 7f9da22 to 362353a Compare March 19, 2019 21:07
Copy link
Member

@mikepizzo mikepizzo left a comment

Choose a reason for hiding this comment

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

:shipit:

@xuzhg xuzhg merged commit fea5363 into OData:master Mar 21, 2019
@xuzhg xuzhg deleted the JsonWriterBuffer branch May 14, 2019 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for review Use this label if a pull request is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants