Only create context if an event is set

[Kahbazi]

Only create context if an event is set

There are multiple events in CookieAuthenticationHandler and each has its own context type which some of them are created per request even when there's no event has been set. We could avoid creating the context if the event has been set.

Description

I did OnValidatePrincipal for start. I will do the same for other events when the solution is accepted.

Fixes https://github.com/dotnet/aspnetcore/issues/42325

[ghost]

Thanks for your PR, @Kahbazi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[Kahbazi]

Ping @Tratcher, in case draft PRs won't send notification!

[Kahbazi]

@davidfowl Please also take a look at this. If you agree with what I've done, I'll do the same for other events as well.

[Tratcher]

This is going to make the code a lot messier and would apply to all the auth handlers, not just cookies. I'm not sure it's worth it. Have you taken profiles of the cookie auth flow? I expect there are more impactful ways we could reduce allocations with less churn.

[Kahbazi]

This is going to make the code a lot messier and would apply to all the auth handlers, not just cookies.

Yeah I agree, that was the reason I only did it for one event. I guess we could make it less messy with moving it to a local function which return the parameters as a Tuple. 🤔

I'm not sure it's worth it.

How about we do it only in HandleAuthenticateAsync method which is the hot path, and is probably run on every request.

Have you taken profiles of the cookie auth flow?

No, not yet. But I'm curious is there something in this change that you think it would make it worse, or you want to see how much is the benefit?

I expect there are more impactful ways we could reduce allocations with less churn.

I will look for them 🔎

[Tratcher]

Have you taken profiles of the cookie auth flow?

No, not yet. But I'm curious is there something in this change that you think it would make it worse, or you want to see how much is the benefit?

I don't think this will make it worse, but I also don't think it will make it much better. It's a lot of code for one allocation.

[Tratcher]

Closing this for now. We'll revisit after profiling.