Make private & internal stuff sealed

[Henr1k80]

No description provided.

[mgravell]

There's a part of the discussion missing here perhaps - what is the motivation for this? Sure, we can do it and it doesn't change the API, but: is there something specific we're doing it for? Note that none of this seems to be likely to be areas where the JIT will care to do anything different for sealed (and even if it did: nothing there is a CPU hot path).

[Henr1k80]

It results in less IL, as the virtual dispatch is no longer needed, and makes the functions candidates for inlining:
dotnet/runtime#49944

The exception is .NET Framework, where it has no effect.

I admit that I have not done any performance analysis of this, but the motivation is correctness and presumed that any performance gains would be welcome, albeit probably hardly measurable.
The sealed keyword is already in heavy use elsewhere, so it can as well be streamlined, maybe even consider adding CA1852

[Henr1k80]

One more thing, it removes the jmp, where the branch predictor has to guess where to load the next instructions from.
If there is a mispredict, the speculative loaded code & data must be discarded and correct code & data loaded, a pipeline stall.
Pipeline stalls are more expensive than the few instructions added by a virtual dispatch.

Branch predictors are very good, but by removing the jmp, there is one less branch to track.
Branches to track is not only in this code, but in the entire application using this library.
The more the branch predictor has to keep track of, the worse the performance: https://blog.cloudflare.com/branch-predictor/

[mgravell]

I'm very aware of the potential advantages of devirtualization; however, whether such things are useful is hugely contextual. Yes, there are some tight loop CPU bound scenarios that might be meaningful in hot code paths, but unless I'm very mistaken, none of that is remotely relevant to any of the situations presented. When suggested arbitrarily without concrete applicable reasons, this ... just seems ... cargo cult programming. I don't mean to be dismissive or disparaging - but I'm also not a fan of changes without demonstrable reasons.

[Henr1k80]

I think the comprehensional cost of adding the sealed keyword is minimal.
So little, that it is not worth proving that it makes a difference.
The IL is smaller, has no branches and makes it possible to inline. Do it always, no discussion needed.
It is not like I am introducing mediator pattern or something, there is not added any new code.

If just a single virtual dispatch can be avoided for a call to redis, it is worth it, to avoid polluting the branch predictors & caches with unneeded stuff.
Scale it with the popularity of this library worldwide.
I expect the libraries I use to be as optimal as can be.

[mgravell]

no discussion needed.

That's not how anything works.

The IL is smaller, has no branches and makes it possible to inline

The IL is identical; it emits the same callvirt in all cases; any devirtualization is done at the JIT, and is dependent on many factors, and I'm pretty sure those rules mean any optimizations here will never apply. Likewise, since you are leaning on "optimal": even if the JIT changes did apply: it is irrelevant to performance here - similar to my comments here.

Note also that the JIT can often sees through virtual without needing the sealed change.

I guess it comes back down to:

There's a part of the discussion missing here perhaps - what is the motivation for this?

The conversation so far has focused on "optimal"; this change will have zero effect on that (or so close to zero that it is negligible). That means we're left with the implicit unstated reason (one of):

• because (some tool) told me to add sealed
• because it can be sealed

These are probably presumably more honest. To quote Phil Davis (White Christmas):

Well, it's not good, but it's a reason.

I do think it is important to be clear about why we're proposing / making any change, and the impact we expect it to have.

[Henr1k80]

Apologies, I meant JIT Asm, not IL, and I realize now that the classes are not used in a way where their exact type is known.

Here is an example on how they should be used, to avoid the virtual dispatch, the C+D Call Asm does not have the dispatch.
I had to trick the compiler, to just not find the sum 18 at compile time and print that.
It still finds the sum of the known classes at compile time, sealed or not, and adds that to the result of the calls to the "unknown" types.

So I agree that it will not make any difference, it is always the base classes that are called, not the exact types, so virtual dispatch will have to be made.

IMHO I still think it is easier to add sealed always, than to do this analysis. I can see that some of the unit test classes follows this philosophy 😜

[mgravell]

I haven't intended to come off as disparaging, and I apologise if I have. This is absolutely the kind of change I would make in passing without much thought or care. I think it's fine to merge, but I think we also need to have a clear idea of what outcome we expect from it, which is going to be mostly: purism. And that's fine - we do that with a bunch of stuff. Heck, going further: layout and typos - all good. I appreciate the input.

[WeihanLi]

maybe better to enable the analyzer on editorconfig also

[mgravell]

PRs welcome ;p