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

Lack of devirtualisation on Bindable<T>.Value #4995

Closed
smoogipoo opened this issue Jan 19, 2022 · 7 comments
Closed

Lack of devirtualisation on Bindable<T>.Value #4995

smoogipoo opened this issue Jan 19, 2022 · 7 comments
Labels
area:bindable priority:1 Very important. Feels bad without fix. Affects the majority of users. type:performance

Comments

@smoogipoo
Copy link
Contributor

We are losing a considerable amount of performance due to Bindable<T>.Value being virtual. Making BindableInt override + seal works however I'd rather push towards improving the bindable structure further instead since this sounds quite limiting.

Current:

Method Mean Error StdDev Allocated
BenchmarkGetValue 16.25 us 0.010 us 0.009 us -

With a sealed override:

Method Mean Error StdDev Allocated
BenchmarkGetValue 2.871 us 0.0231 us 0.0193 us -

With referencing the field directly:

Method Mean Error StdDev Allocated
BenchmarkGetValue 2.837 us 0.0548 us 0.0539 us -
@peppy
Copy link
Sponsor Member

peppy commented Jan 19, 2022

I think there's quite a few scenarios we need to use sealed, too.

@peppy peppy added the priority:1 Very important. Feels bad without fix. Affects the majority of users. label Jan 19, 2022
@smoogipoo
Copy link
Contributor Author

smoogipoo commented Jan 19, 2022

The bindable interfaces are also troublesome, since every interface method access is a virtual call regardless of whether we devirtualise Value.

C# doesn't yet have interface devirtualisation:

Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
BenchmarkNormalClass 2.859 us 0.0013 us 0.0012 us - - - -
BenchmarkNormalClassAsInterface 21.643 us 0.0011 us 0.0010 us - - - -
BenchmarkClassWithVirtual 13.534 us 0.0014 us 0.0013 us - - - -
BenchmarkClassWithVirtualAsInterface 21.644 us 0.0006 us 0.0005 us - - - -
BenchmarkSealedClass 2.855 us 0.0110 us 0.0097 us - - - -
BenchmarkSealedClassAsInterface 21.661 us 0.0276 us 0.0230 us - - - -
BenchmarkStruct 2.711 us 0.0005 us 0.0005 us - - - -
BenchmarkStructAsInterface 35.178 us 0.0207 us 0.0173 us - - - -

Perhaps a ReadOnlyBindable class is something to consider.

@turbedi
Copy link
Contributor

turbedi commented Jan 20, 2022

I tried your benchmark on my machine with .NET 6, but also Dynamic PGO enabled:

Method Job Mean Error StdDev Median
BenchmarkNormalClass Default mode 2.400 μs 0.0139 μs 0.0123 μs 2.402 μs
BenchmarkNormalClass Dynamic PGO 2.422 μs 0.0102 μs 0.0096 μs 2.422 μs
BenchmarkNormalClassAsInterface Default mode 18.929 μs 0.0435 μs 0.0386 μs 18.923 μs
BenchmarkNormalClassAsInterface Dynamic PGO 5.174 μs 0.0041 μs 0.0037 μs 5.174 μs
BenchmarkClassWithVirtual Default mode 14.211 μs 0.0287 μs 0.0255 μs 14.207 μs
BenchmarkClassWithVirtual Dynamic PGO 5.089 μs 0.1001 μs 0.1644 μs 5.167 μs
BenchmarkClassWithVirtualAsInterface Default mode 18.884 μs 0.0326 μs 0.0272 μs 18.880 μs
BenchmarkClassWithVirtualAsInterface Dynamic PGO 5.110 μs 0.0997 μs 0.1430 μs 5.166 μs
BenchmarkSealedClass Default mode 2.399 μs 0.0140 μs 0.0131 μs 2.396 μs
BenchmarkSealedClass Dynamic PGO 2.419 μs 0.0055 μs 0.0046 μs 2.421 μs
BenchmarkSealedClassAsBase Default mode 14.271 μs 0.0366 μs 0.0342 μs 14.291 μs
BenchmarkSealedClassAsBase Dynamic PGO 5.125 μs 0.1056 μs 0.1335 μs 5.178 μs
BenchmarkSealedClassAsInterface Default mode 18.998 μs 0.0197 μs 0.0164 μs 19.004 μs
BenchmarkSealedClassAsInterface Dynamic PGO 4.769 μs 0.0048 μs 0.0037 μs 4.769 μs
BenchmarkStruct Default mode 2.417 μs 0.0165 μs 0.0154 μs 2.420 μs
BenchmarkStruct Dynamic PGO 2.413 μs 0.0093 μs 0.0082 μs 2.416 μs
BenchmarkStructAsInterface Default mode 32.995 μs 0.0312 μs 0.0291 μs 32.990 μs
BenchmarkStructAsInterface Dynamic PGO 4.775 μs 0.0244 μs 0.0216 μs 4.770 μs

According to the post Dynamic PGO can also cause regressions in some cases, but this is supposed to be fixed in .NET 7

@smoogipoo
Copy link
Contributor Author

Good to know. I believe in general we can turn on Dynamic PGO with .NET 6 already, did some rough benchmarks as such a while back.
Will need to see if there are runtimeconfig flags for it, or if we'd need to wait for it to be turned on by default.

@turbedi
Copy link
Contributor

turbedi commented Feb 2, 2022

Runtime configs are planned for .NET 7

@Rekkonnect
Copy link
Contributor

Since the project was upgraded to .NET 8.0, maybe revisiting this is a good idea

@smoogipoo
Copy link
Contributor Author

Dynamic PGO is enabled by default in .NET 8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:bindable priority:1 Very important. Feels bad without fix. Affects the majority of users. type:performance
Projects
None yet
Development

No branches or pull requests

4 participants