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
Reduce allocations in schedulers #500
Reduce allocations in schedulers #500
Conversation
@@ -18,6 +19,7 @@ | |||
<None Update="Platforms\Windows\Strings_WindowsThreading.resx" CustomToolNamespace="System.Reactive" Generator="ResXFileCodeGenerator" LastGenOutput="Strings_WindowsThreading.Designer.cs" /> | |||
</ItemGroup> | |||
<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard1.3' "> | |||
<PackageReference Include="System.ValueTuple" Version="4.4.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused. Has this become a standard or is this just a library? I can remember this was mentioned in some C# what's new/upcoming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a library that is needed for most targets to enable use of C# value tuples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a feature of C# 7.
} | ||
|
||
private static IDisposable ScheduleAsync_<TState>(IScheduler scheduler, TState state, Func<IScheduler, TState, CancellationToken, Task> action) | ||
{ | ||
return scheduler.Schedule(state, (self, s) => InvokeAsync(self, s, action)); | ||
return scheduler.Schedule((state, action), (self, t) => InvokeAsync(self, t.state, t.action)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it have to be tuples? Can't you just define a new struct to hold both parameters and avoid the hassle with changing the language version and depending on yet another external library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C# 7.1 aims to avoid exactly that, having to create structs everytime you need such tuples. Besides, the language support for tuples is really nice, the syntax is very low-noise. What exactly do you mean by 'hassle with changing the language version' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't certain C# language versions require minimum .NET Framework, .NET Standard or .NET Core versions to run? https://en.wikipedia.org/wiki/C_Sharp_(programming_language)#Versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if so, where's the break from C# 7.0 to 7.1? I didn't even pull it up to 7.3 right away, which would probably be fine as well.
// Once Roslyn supports caching delegates for method groups, | ||
// the anonymous lambda can be replaced by the method group again. Until then, | ||
// to avoid the repetition of code, the call to Invoke is left intact. | ||
// Watch https://github.com/dotnet/roslyn/issues/5835 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general note, I'd think Rx 4.0 should run on wide variety of JITs and runtimes, most of which won't be advanced enough to cache things for you. Wouldn't it be safer to do these by hand so all of the targets get the optimal code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a missing compiler feature, not a runtime feature. Doing this by hand? Really?
9bb545e
to
81fc53c
Compare
ad55797
to
13130f8
Compare
c2dcf49
to
0ca08f5
Compare
…currency namespace.
0ca08f5
to
5c6e43e
Compare
The infinitive generics recursion interacts poorly with AOT. The AOT compilers have hard figuring out where the stop generating the code for generics with infinite recursion. They either fail or produce large images by giving up once the generics get too complex. This change reverts a small part of dotnet#500 that introduced infinite generic recusion and adds comment. Fixes dotnet/corert#7920
The infinitive generics recursion interacts poorly with AOT. The AOT compilers have hard figuring out where the stop generating the code for generics with infinite recursion. They either fail or produce large images by giving up once the generics get too complex. This change reverts a small part of dotnet#500 that introduced infinite generic recursion and adds comment. Fixes dotnet/corert#7920
The infinitive generics recursion interacts poorly with AOT. The AOT compilers have hard figuring out where the stop generating the code for generics with infinite recursion. They either fail or produce large images by giving up once the generics get too complex. This change reverts a small part of #500 that introduced infinite generic recursion and adds comment. Fixes dotnet/corert#7920
The infinitive generics recursion interacts poorly with AOT. The AOT compilers have hard figuring out where the stop generating the code for generics with infinite recursion. They either fail or produce large images by giving up once the generics get too complex. This change reverts a small part of #500 that introduced infinite generic recursion and adds comment. Fixes dotnet/corert#7920
Some allocations of closures and delegates can be avoided, which should be very effective in code that uses Schedulers a lot. Note that this PR increases the language version used to C# 7.1 and takes a reference to System.ValueTuple for every target except .netstandard 2.