aws / aws-sdk-net

The official AWS SDK for .NET. For more information on the AWS SDK for .NET, see our web site:
http://aws.amazon.com/sdkfornet/
Apache License 2.0
2.07k stars 861 forks source link

Take advantage of .NET Core 2.1 Span<T> Memory<T> Channel<T> MemoryPool Buffers, etc #971

Closed jarrettv closed 3 years ago

jarrettv commented 6 years ago

The intent of this issue is to jumpstart a discussion on ways the aws-sdk-net can take advantage of the all the goodness added in .NET Core 2.1.

I've seen there are many hashing, byte arrays, streams, etc that on the surface look like they could benefit. There should be a large improvement just from the new HttpClientHandler.

Your thoughts?

slang25 commented 6 years ago

I've had the same thought, and would be willing to help contribute towards this.

There are 2 ways to use Span<T> and family, first is using it between your own code, and second is being able to pass them into the framework methods that support them, for that you need to target netcoreapp2.1, until the next netstandard comes along, and that's not going to be soon.

If the latter were to be done, you'd start by multi-targeting netcoreapp2.1 in addition to netstandard1.3, and off you go šŸ˜ƒ

normj commented 6 years ago

Love to have the conversation about this. I agree Span for all our JSON parsing could do wonders for performance. We currently use an embedded version LitJson instead of the common Newtonsoft to avoid locking anybody to a specific version of Newtonsoft. Plus LitJson is very small compared to Newtonsoft.

I don't think HttpClientHandler would buy us too much. We try to abstract away the underlying HttpClient because it changes depending on the platform. The main benefit it would give the SDK is we could replace our own HttpClient caching code. The new implementation of HttpClient should be really helpful in non-windows environments but I haven't had time yet to quantify that.

The trickiest part of these updates is that they are not part of any .NET Standard yet and also the AWS SDK is currently targeting .NET Standard 1.3. We have had some conversations of doing a semi major version bump where we would move the SDK to .NET Standard 2.0 and drop .NET Framework 3.5, yes we still supported 3.5.

slang25 commented 6 years ago

Supporting .NET 3.5 comes at a big cost, out of interest, is there a demand for new AWS features on that runtime?

akatz0813 commented 6 years ago

Has anyone done any performance testing of the library when running on 2.1 versus 2.0, especially on linux? The default HttpClient library is now by default sockets, so simply upgrading the runtime should hopefully deliver some immediate short term wins.

normj commented 6 years ago

@slang25 We haven't done any new features for .NET 3.5 in quite sometime but the service clients are generated and as part of the generation it does produce 3.5 compliant versions with the old BeginXXX and EndXXX async pattern. You are right it does come with a cost which is why we are debating if it is time to move past it.

indy-singh commented 6 years ago

I've seen there are many hashing, byte arrays, streams, etc that on the surface look like they could benefit.

Agreed. I would love to see more of .NET Core goodness, perhaps even System.Buffers and Microsoft.IO.RecyclableMemoryStream that I mentioned as possible solutions in https://github.com/aws/aws-sdk-net/issues/894.

Regards, Indy

hmvs commented 6 years ago

@akatz0813 Actually I am profiling our app right now. App is taking something like 60k incoming requests per minute and aggregates them into batches and sending them into firehose). And I can see that after upgrading ALL the packages into 2.1. I have huge performance hit. I am trying to understand what is the cause of this performance hit but have no luck yet.

danmoseley commented 6 years ago

@hmvs did you narrow down your perf issue? 2.1 should obviously be faster.

hmvs commented 6 years ago

Not yet. @sebastienrosĀ is working on it.

akatz0813 commented 6 years ago

@hmvs and @sebastienros we are seeing the same issue on web endpoints, apparently due to Kestrel sockets. We had to switch back to libuv.

There is a similar issue to our behavior. https://github.com/aspnet/KestrelHttpServer/issues/2621

damianh commented 6 years ago

Am fairly confident that nobody would would miss net35 if it were dropped. You could also drop PCL and possiblyUnity. Keep net45 (as net452 is still officially supported) and netstandard13. Introduce netstandard20 and support all via a single multi-targeted project too. To use netcoreapp2.1 features would likely need conditional compilation (/separate projects) at this time.

To be honest the solution is a bit of a beast atm and am finding it hard to develop a contribution give the complexity of the many solutions and projects. A clean up would be excellent.

Cheers.

normj commented 6 years ago

@damianh What you are suggesting is where I want to get the SDK.

I agree the solutions in the SDK repository are very difficult to work with even for us. It is something I want the team to work on to make it easy for anybody to contribute to the SDK. As you can imagine there is a lot of automation behind the scenes with the SDK to drive the daily release cycle of the SDK. We are slowly making changes to the repository but we have to do that in tandem with updating the automation systems and keep everything running for the daily release.

gitfool commented 5 years ago

@normj I'd also love to see a netstandard20 target for all the AWS SDK packages. Any progress?

sstevenkang commented 5 years ago

Yup! We just added netstandard20 target to our NuGet packages. All AWS .NET SDK NuGet packages starting with 3.3.100 should support .NET Standard 2.0.

See our announcement here!

ashishdhingra commented 3 years ago

Hi @jarrettv,

Good afternoon.

I was going through the backlog and came across this issue. Please confirm if all the points in the discussion are addressed and if this issue could be closed due to lack of activity.

Thanks, Ashish

github-actions[bot] commented 3 years ago

This issue has not recieved a response in 2 weeks. If you want to keep this issue open, please just leave a comment below and auto-close will be canceled.