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

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

Closed
jarrettv opened this issue Jun 1, 2018 · 16 comments
Labels
closed-for-staleness feature-request A feature should be added or improved. module/sdk-core response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@jarrettv
Copy link

jarrettv commented Jun 1, 2018

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
Copy link
Contributor

slang25 commented Jun 1, 2018

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
Copy link
Member

normj commented Jun 1, 2018

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
Copy link
Contributor

slang25 commented Jun 1, 2018

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

@akatz0813
Copy link

akatz0813 commented Jun 1, 2018

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
Copy link
Member

normj commented Jun 1, 2018

@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
Copy link
Contributor

indy-singh commented Jun 9, 2018

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 #894.

Regards,
Indy

@hmvs
Copy link

hmvs commented Jun 10, 2018

@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
Copy link

danmoseley commented Jun 23, 2018

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

@hmvs
Copy link

hmvs commented Jun 29, 2018

Not yet. @sebastienros is working on it.

@akatz0813
Copy link

akatz0813 commented Jun 29, 2018

@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. aspnet/KestrelHttpServer#2621

@damianh
Copy link
Contributor

damianh commented Jul 19, 2018

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
Copy link
Member

normj commented Jul 19, 2018

@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
Copy link

gitfool commented Nov 26, 2018

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

@sstevenkang
Copy link
Contributor

sstevenkang commented Mar 22, 2019

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 ashishdhingra added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jul 9, 2020
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Feb 17, 2021

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

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Feb 17, 2021
@github-actions
Copy link

github-actions bot commented Mar 4, 2021

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.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Mar 4, 2021
@github-actions github-actions bot added closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness feature-request A feature should be added or improved. module/sdk-core response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests