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

Add a system.text.json formatter #305

Closed
AArnott opened this issue Jul 15, 2019 · 5 comments · Fixed by #908
Closed

Add a system.text.json formatter #305

AArnott opened this issue Jul 15, 2019 · 5 comments · Fixed by #908
Assignees
Milestone

Comments

@AArnott
Copy link
Member

AArnott commented Jul 15, 2019

The new System.Text.Json APIs are much faster than Newtonsoft.Json, and thus offer a huge perf boost without sacrificing interop as a move to MessagePack has.

Be sure when doing so to implement full streaming support such that we don't buffer an entire message before beginning deserialization.

@AArnott
Copy link
Member Author

AArnott commented Nov 26, 2019

No demand for this, so closing.

@AArnott AArnott closed this as completed Nov 26, 2019
@watfordgnf
Copy link

I realize this is resurrecting an old closed issue, but we would be interested in a System.Text.Json version.

We've got a job scheduler that has coordinators on nodes managing dozens of sub-processes, talking over anonymous pipes with StreamJsonRpc. The JsonArrayPool class causes rather large char arrays to be pinned when we ship logs back and forth. We've temporarily removed log shipping over StreamJsonRpc until we can figure out how to avoid this.

@AArnott
Copy link
Member Author

AArnott commented Sep 17, 2020

Hi @watfordgnf. Thanks for reaching out. What is JsonArrayPool? Is that a newtonsoft.json class? Is the issue that you're sending large strings and newtonsoft.json caches those large strings as char arrays?

If you're sending large data like that, have you considered using Stream arguments to pass that data? It would bypass newtonsoft.json and JSON-encoding of your text files entirely for that data so it would be much more efficient in a few ways.

@watfordgnf
Copy link

Yeah it is used by Newtonsoft.Json:

/// <summary>
/// Adapts the .NET <see cref="ArrayPool{T}" /> to Newtonsoft.Json's <see cref="IArrayPool{T}" /> interface.
/// </summary>
private class JsonArrayPool<T> : IArrayPool<T>
{
private readonly ArrayPool<T> arrayPool;
internal JsonArrayPool(ArrayPool<T> arrayPool)
{
this.arrayPool = arrayPool ?? throw new ArgumentNullException(nameof(arrayPool));
}
public T[] Rent(int minimumLength) => this.arrayPool.Rent(minimumLength);
public void Return(T[] array) => this.arrayPool.Return(array);
}

Currently the log data is not streamed and is a string (downstream dependency), but I guess if we wrapped that in a MemoryStream then we could utilize the Stream argument instead.

@AArnott
Copy link
Member Author

AArnott commented Sep 17, 2020

Great. Using Stream sounds like it will resolve your issue more quickly than waiting for System.Text.Json support in this library, which has no timeline at this point.
But another good idea (can you open a new issue for this?) would be for the JsonMessageFormatter to more aggressively limit the size of the arrays it stores in the pool it uses than whatever the standard ArrayPool<T>.Shared setting is. Please mention in that new issue what sizes you're seeing stored there so we can take that into account when choosing a better limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants