Cleanup permission providers

[MikeAlhayek]

@Piedone In this PR I like to take the cleanup done in #15089. Much cleaner than before and consistent across the broad.

It is hard to see the actual difference here because of the namespace scope change. The main difference is that we create an instance on the collection to use, and we use it in the same class so we don't need to create multiple instances "small performance gain and less allocation" For example

OrchardCore/src/OrchardCore.Modules/OrchardCore.Admin/Permissions.cs

Line 11 in b16cbbe

private readonly IEnumerable<Permission> _allPermissions =

[github-actions]

This pull request has merge conflicts. Please resolve those before requesting a review.

[Piedone]

Why does this make sense when #15089 didn't? It seems to me that this does a similar kind of optimization but at a smaller scale (fewer objects cached for shorter).

[MikeAlhayek]

@Piedone in #15089 I used static properties to store the instance which was not good because the object will live for the life of the app. In this case, it is not static.

[Piedone]

I understand, that's what I meant by those fewer objects being cached shorter. However, under the other PR our consensus was that it's not worth it since the providers are not actually called that many times. Why does it make sense to do a smaller optimization, then?

[MikeAlhayek]

Because I already did the cleanup work in the PR. + extra small optimization won't hurt.

[Piedone]

As far as I see, the changes here, apart from file-scope namespaces and some smaller formatting changes (I agree with these), are all that small optimization (I'm not sure I'd call it clean-up, I don't think the original implementation had anything unclean about it; perhaps a matter of taste). But then again, why does it make sense to optimize little if optimizing big wasn't worth it?

[MikeAlhayek]

Why no optimize if you can help it? creating extra object and adding them to the Heap just because is not ideal either. These handlers are still called many times (during loading the Roles edit page, during editing the Admin menu nodes, when the site is created and also every we release shell (enable feature or update settings). So instead of have to create multiple objects just because, we can cut down that and reduce the size of the allocated objects in the heap. Sure it is not a whole lot of saving, but reduce the load of the heap. Keep in mind all the objects are gets allocated at some point will have to be garbage collected. So if there is a way to reduce allocations, why not. Do you see an issue with the PR?

[Piedone]

I agree with you. However, why don't we merge #15089, then? That's all I'm asking. I'm not trying to be an opposition just for itself. I merely try to understand, that if the optimization changes in this PR (not talking about the formatting) are worthwhile, then why aren't the ones under the other PR? (Which is much of the same plus some static.) Please educate me. I'm not trying to be cynical.

[MikeAlhayek]

@Piedone if you are asking why not utilize the previous PR. Because the original intention was completely different. Originally, the idea was to use static instances of permissions collections and stereotype collection so that we can utilize them across all tenants. But, after digging, we realized it was not a good idea since these objects are not initialized on every request. Additionally, there was some formatting changes for consistency across the board.

So in this PR, I took most of what we did in the other PR, but dropped the idea of using static instance that live for the duration of the app.

Since the other PR had lots of communication, and to eliminate confusion I created a new PR with different intention and to eliminate the confusion.

I hope that answer your question.

[Piedone]

Cool, thanks!