Skip to content

C#: Various performance fixes #14086

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

Merged
merged 4 commits into from
Aug 30, 2023
Merged

C#: Various performance fixes #14086

merged 4 commits into from
Aug 30, 2023

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Aug 29, 2023

Commit-by-commit review is encouraged. These performance fixes make no difference on the projects we have in DCA, but they make a big difference on a particular customer database (see tuple counts in commit messages).

Avoids the following bad predicate

```
[2023-08-29 10:03:13] (252s) Tuple counts for _Callable#f85cebf6::Callable::getBody#0#dispred#ff_Variable#afb43847::Variable::getAnAccess#0#dispre__#join_rhs/5@43feb6tl after 4m0s:
                      4416261    ~203%     {4} r1 = JOIN _Callable#f85cebf6::Callable::getAParameter#0#dispred#ff_10#join_rhs_Variable#afb43847::Variable::ge__#shared WITH Callable#f85cebf6::Callable::getBody#0#dispred#ff ON FIRST 1 OUTPUT Lhs.1 'arg1', Lhs.2 'arg2', Lhs.0 'arg3', Rhs.1 'arg4'
                      1189565718 ~152%     {5} r2 = JOIN r1 WITH Variable#afb43847::Variable::getAnAccess#0#dispred#ff ON FIRST 1 OUTPUT Rhs.1 'arg0', Lhs.0 'arg1', Lhs.1 'arg2', Lhs.2 'arg3', Lhs.3 'arg4'
                                           return r2
```
```
[2023-08-29 10:10:29] Evaluated non-recursive predicate SsaImpl#75014cd4::Cached::lastRefBeforeRedefExt#4#ffff@4207c208 in 27604ms (size: 7511062).
Evaluated relational algebra for predicate SsaImpl#75014cd4::Cached::lastRefBeforeRedefExt#4#ffff@4207c208 with tuple counts:
           9905038   ~9%    {5} r1 = SCAN Ssa#da392372::Make#SsaImpl#75014cd4::SsaInput#::lastRefRedefExt#5#fffff OUTPUT In.2, In.3, In.1, In.0, In.4
                            {5} r2 = r1 AND NOT _SsaImpl#75014cd4::SsaInput::variableRead#4#ffff_3012#join_rhs_const_false#antijoin_rhs(Lhs.0, Lhs.1, Lhs.2)
           4605608   ~0%    {4} r3 = SCAN r2 OUTPUT In.3, In.0, In.1, In.4

        4510888816   ~0%    {5} r4 = JOIN _SsaImpl#75014cd4::SsaInput::variableRead#4#ffff_3012#join_rhs_const_false#antijoin_rhs WITH project#Ssa#da392372::Make#SsaImpl#75014cd4::SsaInput#::lastRefRedefExt#5#fffff_1203#join_rhs ON FIRST 2 OUTPUT Rhs.2, Lhs.2, Lhs.0, Lhs.1, Rhs.3
           5294405  ~82%    {4} r5 = JOIN r4 WITH SsaImpl#75014cd4::adjacentDefReachesReadExt#6#ffffff_014523#join_rhs ON FIRST 4 OUTPUT Lhs.0, Rhs.4, Rhs.5, Lhs.4

           9900013  ~28%    {4} r6 = r3 UNION r5
                            return r6
```
@github-actions github-actions bot added the C# label Aug 29, 2023
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Aug 29, 2023
@hvitved hvitved marked this pull request as ready for review August 29, 2023 18:12
@hvitved hvitved requested a review from a team as a code owner August 29, 2023 18:12
Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!
Only comment is increased the closed-world for dispatch on type parameters. Have you checked for cases where this causes zero dispatch targets and whether that's desirable? Also, have you e.g. done a MRVA run to gauge the widespread impact this has on virtual dispatch? For comparison, Java generally applies an open-world assumption when it comes to dispatch on type parameters so I'm curious about the trade-off here and whether we're loosing some needed dispatch targets. Looks like there's already a closed-world assumption in place for unconstrained type parameters, so these questions also applies somewhat to the current situation on main.

@hvitved
Copy link
Contributor Author

hvitved commented Aug 30, 2023

Only comment is increased the closed-world for dispatch on type parameters.

It is limited to calls involving reflection/dynamic, so I think it is OK to use closed-world assumption in that case.

@aschackmull
Copy link
Contributor

It is limited to calls involving reflection/dynamic, so I think it is OK to use closed-world assumption in that case.

Ah, ok. Merge away then!

@hvitved hvitved merged commit c32c4bb into github:main Aug 30, 2023
@hvitved hvitved deleted the csharp/perf-fixes branch August 30, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants