Cleanup Permission Providers to reduce allocations

[MikeAlhayek]

@sebastienros this should reduce allocation during tenant startup and enabling features.

[Piedone]

Did you do some before-after measurements?

[MikeAlhayek]

No. I actually did not. I think to know for sure we would have to run OC using multiple tenants and test it out. But with this PC, we are creating constants for objects so we don't have to create new object every single time. Creating objects will have to be placed on the heap and then released later during GC. Since these are constants, they are allocated once's for all of the tenants and features. So when a tenant enables disables and feature or the tenant shell is released, we don't have to create the same objects every time. Also, it clean up our permission providers

[Piedone]

I understand the argument, and I also would expect lower memory consumption and/or less CPU usage due to less GC. However, long-lived objects instead of ones quickly garbage collected can be an issue. I'm not sure though, and as with performance fixes usually, I think we need to test it to see if actually helps or for some reason that we didn't think of it doesn't.

[MikeAlhayek]

@sebastienros is there any metrics we can use with Crank to tell if this is a good approach or not? If we have tenants that don't rebuild their container (no settings changes, and no feature state changes), then I would say this is NOT ideal. However, for apps with many tenants and frequent shell releases, this should be beneficial. I am not sure if there is a way to test this out using Crank or any other tool. Thought?

[sebastienros]

Add some log in GetDefaultStereotypes() and GetPermissionAsync() in a few classes that are enabled. Do a front-end anonymous request on a blog page and count how many times it's used. Do the same with an authenticated request on the admin, like when editing a content item.

If these methods are only called on startup then I agree with Zoltan that it's detrimental. If these methods are called for each request then I believe it's worth it.

[Piedone]

If there's a NuGet package then we can also test with DotNest that has exactly the usage pattern you mention, Mike. But that'll take a while.

[MikeAlhayek]

@sebastienros @Piedone Here are my findings. We use IEnumerable<IPermissionProvider> in 3 places.

1. RoleUpdater which used only when we enable/disable feature.
2. AdminController on the roles which is used when we render the permissions on the UI.
3. We also use it in the AdminMenuPermissionService which is called on every request when the user edits the AdminMenu nodes.

So we do not evaluate these these permissions all the time, but we evaluate them in key UIs.

[sebastienros]

Then it's not worth it.

[Piedone]

Yeah it looks like that. Authorization calls are not calling these in the end, right?

[MikeAlhayek]

It does not.

[Piedone]

I see, then indeed not worth it.