-
-
Notifications
You must be signed in to change notification settings - Fork 493
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 async support for all IO operations #430
Comments
I wonder what the performance benefit would actually be here, since in my experience this is already pretty quick! Would you create a second set of functionality with the Async suffix? Or would this be an upgrade and replace of all non async functionality to be async? |
The intention is more to be able to support workflows that only allow async operations. For example, the aspnet request or response streams, which by default require async. I don't have a clear design in mind yet, but I think that the synchronous API would still be necessary, therefore there would need to be a second set of APIs with the async suffix. Forcing async usage in all cases would have a performance cost for all memory-only use cases, which I think are relatively common. |
I like your plan, it makes sense. If you want to identify particular functions that you'd like to convert, I'm happy to contribute and work on this. I have some experience writing async functions... |
Sure. At the moment I can't dedicate too much time to this feature, so any help is welcome! |
I've created an initial commit and would value your input. Would you mind having a peak to make sure it meets your standards before I continue? Note that this is only for "serialize" so far, and I've only written one async test (I tried to write an alternative for a (relatively) long running unit test, which you can see, appears to have a significant performance boost): Pull Request: #457 (build currently failing due to some performance test issue I will also investigate) |
I took a look at picking up #457 -- I agree with the comment there that
Personally, those paths are in order of preference and it's likely I could devote some significant development time towards 1 or 2. |
Regarding the options that you suggest, I think that only the first one is viable. Option 2 would add very little value, I think, and if there was an easy way to implement option 3, it would probably already be built into the C# language. Not keeping the synchronous API is a problem because we target platforms that do not support async. I'm reluctant to discontinue their support and would rather not have async APIs if this proved too difficult to maintain. I have another concern which is that the emitter currently performs a lot of small writes to the underlying TextWriter. Most of them will probably end up just writing to a buffer and return synchronously. This means that we'll end up having a lot of overhead and maybe the end result will be worse than using the synchronous APIs... We would need to perform some tests before fully committing to using async. I believe that using System.IO.Pipelines could help here but would require a complete reimplementation of the parser and emitter. Do you have ideas to address these issues ? |
I agree with everything stated above. I started to look at how other serializers had handled this. It looks like Newtonsoft.Json hasn't fully implemented async support -- they seem to have support in the parser but not in the serializer. I then took a look at They seem to be calling the normal synchronous code and then just copying the output to the stream asynchronously. This sort of approach would seem to be a lot easier to implement in YamlDotNet but I'm not sure how much it really improves performance? I may try and throw together a test of that approach in the next couple of days to see. |
Regardless of performance with more conventional streams, the |
Just came across https://devblogs.microsoft.com/dotnet/system-io-pipelines-high-performance-io-in-net/ which seems highly applicable here (and I now realize you pointed out in your last comment). |
Yes, this would probably be the way forward, although we would have issues with some of the targetted platforms. I don't think that it is very important to keep supporting .NET 2.0, but Unity uses .NET 3.5 and I believe that there are many users of the library on that platform. |
I think with that approach it might be possible to avoid excessive code duplication and use conditional compiles and package references to maintain compatibility. I haven't yet had time to prototype anything concrete to see if it pans out. |
Antoine, do you need help on this? |
I'm not currently working on this, but maybe @willson556 does. I'm currently working on a different feature, schemas, and can't spend time on this feature at the moment. |
I haven't started work on this either. I was sort of waiting to see the outcome of #482 and #486 before proceeding with further contributions though I do have a long-run interest in building this. It is my current belief based on some work we've done since April that it would not be too difficult to do this using the producer/consumer pattern utilizing |
Sounds like fun. How can I help? |
Any news on this topic since 2020? Some .NET parts do block synchronous IO operations, so having an async API here would definitely make things smoother. |
+1 to a high-speed implementation based on |
Add async overloads of all methods that perform IO.
I am opening this ticket because I feel that this should eventually be implemented. If this is a feature that would be valuable to you, please say so in the comments.
The text was updated successfully, but these errors were encountered: