[do not merge] feedback PR for perf improvements

[jennyf19]

No description provided.

[DavidParks8]

TryGetValue results in a single dictionary lookup rather than two lookups when using ContainsKey with subsequent access.

Suggested change

	if (headers.ContainsKey(AppServicesAuthIdTokenHeader))
	if (headers.TryGetValue(AppServicesAuthIdTokenHeader, out var idToken))

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7057 Code Smells

No Coverage information
0.0% Duplication

[DavidParks8]

CurrentHttpContext needs to be locked on to avoid threading issues with external code.

[DavidParks8]

long.parse could throw if the claim has a broken expiration string. long.TryParse would be safer.

[DavidParks8]

there are three null checks on userClaims here.
userClaims?.GetDisplayName()
userClaims?.GetTenantId()
userClaims != null

It would be faster to do the check once and set three variables.

string? displayName;
string? tenantId;
Account? account;
if (userClaims != null)
{
   displayName = userClaims.GetDisplayName();
   tenantId = userClaims.GetTenantId();
   account = new Account(userClaims);
}
else
{
   displayName = null;
   tenantId = null;
   account = null;
}

[DavidParks8]

the async keyword can be removed when all the function does is throw. Doing so would avoid the need to suppress a warning about async functions needing something awaited.

[DavidParks8]

Oh wait, I already did that in this file in my other PR.

[DavidParks8]

This doesn't really matter that much, but the else keyword and block nesting isn't needed because the if (options.IsB2C) block had a return statement.

[DavidParks8]

For strings that are language and culture agnostic, StringComparison.Ordinal should be used. It is faster and more targeted to the usecase here: https://docs.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings

[DavidParks8]

String allocations tend to be a death by 1000 cuts issue at scale. Instead of TrimEnd('/'), I suggest adding "v2.0" or "/v2.0" based upon the presence of the trailing slash. Doing so should skip one of the string reallocations.

[DavidParks8]

The error messages should really be localized in order to promote the most inclusive api experiences.

[DavidParks8]

Once localized, InvariantCulture is probably not the correct culture to use.

[DavidParks8]

GetRequiredService will throw if the service isn't registered. I recommend changing it to GetService to avoid an unhelpful error.

[DavidParks8]

This if block can be less nested by combining towards "else if" which will slightly improve readability.

[DavidParks8]

context.Failure is OpenIdConnectProtocolException can be evaluated once rather than re-evaluated in multiple if statements.

[DavidParks8]

Doing this will be twice as fast:

Suggested change

	}
	if (!_userFlowToIssuerAddress.TryGetValue(userFlow, out var issuerAddress))
	{
	issuerAddress = context.ProtocolMessage.IssuerAddress.ToLowerInvariant()
	.Replace($"/{defaultUserFlow?.ToLowerInvariant()}/", $"/{userFlow.ToLowerInvariant()}/");
	_userFlowToIssuerAddress[userFlow] = issuerAddress;
	}
	return issuerAddress;

[jennyf19]

lol...i had fixed that when doing the ContainsKey fixes.

[DavidParks8]

The title string needs to be localized.

[DavidParks8]

This is a good example of where string.Format isn't needed

Suggested change

	private static readonly string DoubleBase64PadCharacter = string.Format(CultureInfo.InvariantCulture, "{0}{0}", Base64PadCharacter);
	private const string DoubleBase64PadCharacter = Base64PadCharacter + Base64PadCharacter;

[DavidParks8]

Having this be static and externally settable opens up race conditions where someone thinks it is populated when it may not be, and then makes incorrect decisions based upon that. Saying it is not used that way now is not sufficient. This is a high risk of misuse for future contributors. Recommendation is to make it an instance field on a new interface that is not settable externally, then access it through dependency injection.

[jennyf19]

will raise an issue for this.

[jennyf19]

#1095

[DavidParks8]

This sample looks a bit broken with missing end brackets and overwriting of HttpContext.User with each iteration.

[jennyf19]

great eye.

[DavidParks8]

setting to null! seems like we are incorrectly trying to override the type. The reality is that json can omit the field which would result in a null string.

[DavidParks8]

All of these extension methods could be deduplicated into calling a single private shared method:

private static string? GetClaimValue(ClaimsPrincipal? claimsPrincipal, params string[] claimNames)
{
    if (claimsPrincipal == null)
    {
        throw new ArgumentNullException(nameof(claimsPrincipal));
    }

    for (var i = 0; i < claimNames.Length; i++)
    {
        var currentValue = claimsPrincipal.FindFirstValue(claimNames[i]);
        if (!string.IsNullOrEmpty(currentValue))
        {
            return currentValue;
        }
    }

    return null;
}

[DavidParks8]

There are many constants throughout the codebase. It may be worthwhile to rename this class to be more descriptive about the specific constants it holds.

[DavidParks8]

Please consider using LoggerMessage as described at https://docs.microsoft.com/en-us/dotnet/core/extensions/high-performance-logging instead of doing costly message formats into strings.

[DavidParks8]

This code is in the hotpath of requests. As such, it would likely speed up by 5-7x if regular expressions were avoided where possible. We did a similar change in a planet scale service and sped the code up by 7x.

[DavidParks8]

This warning suppression might be able to be removed now that strict null types is enabled.

[DavidParks8]

the response var isn't used outside the scope of the using block. It can be removed and replaced by a direct return statement in front of the awaited call.

[DavidParks8]

Using exceptions for control flow is a very inefficient way of doing things. I recommend instead checking if (!response.IsSuccessStatusCode) and only then throwing the HttpRequestException.

[DavidParks8]

Assuming the input is intended to be default json is something which disallows calling downstream apis using other formats such as msgpack. Msgpack is a popular format used within internal containerized servers that need more performance when communicating amongst each other.

[DavidParks8]

This is clonable, so why not have it implement ICloneable?

[DavidParks8]

to illustrate best practices, the task should be returned instead of awaited in the example.

[DavidParks8]

This can be done faster, but probably shouldn't be. The faster implementation I'm thinking of involves char operations and a trie built from the string collection inputs..

[DavidParks8]

Why is the FullName field parsed rather than using the .Version property of the Assembly object?

[DavidParks8]

LINQ is super slow. Like, so slow, I would recommend people almost never use it when there is a non-verbose alternative. In this case, there is a data structure that perfectly does what we want in an optimized way. That data structure is the HashSet which happens to have a UnionWith method and already implements ICollection.

[DavidParks8]

Can the type be moved to its own file in order to make it easier to find on the github folder view?

[DavidParks8]

Everytime HttpContext is touched, it will need somesort of lock in this code.

[jennyf19]

#1097

[DavidParks8]

This line is a problem because it causes the original stacktrace to be lost. The exception needs to be thrown with throw; instead of throw exception; which means it can't be done inside this method and instead needs to be done from a catch block directly.

[jennyf19]

we are given the exception to process here for incremental consent, not sure how we can move this to a try/catch. otherwise, developers would have to do this, and it becomes too complicated.

[DavidParks8]

ToArray results in an unnecessary allocation.

[DavidParks8]

HttpContext can definitely be null here if the library is attempted to be used within a worker service. With that in mind, the warning should be addressed.

[jennyf19]

this is only for blazorwasm scenarios, so we will always have the httpContext.

[DavidParks8]

IsBlazorServer might not be correct or am I reading this wrong?

[jennyf19]

this is only for blazorwasm...it's a long story.

[DavidParks8]

faults should minimally be logged.

[DavidParks8]

in case someone is trying to break things, the cast would be safer as tenantId as string;

[DavidParks8]

This naming convention doesn't match the rest of the codebase. Historically, s_ meant static, which makes this naming a bit misleading.

[DavidParks8]

This could use the unLINQ treatment.

[DavidParks8]

Can this please be ifdef'd out of existence do it is excluded from the build in release mode? The reflection used in this method makes it very slow.

[jennyf19]

developers have to set it explicitly, and i doubt they would use it in release mode. we can make a comment in the wiki, and we could add an if/def too...sometimes we might need the diagnostics in release mode.

[DavidParks8]

.Any() is much slower than string[].Length > 0;

[DavidParks8]

Why does an exception need to be thrown when the response was already completed in the line above?

[jennyf19]

so the rest of the controller action is not done. if the scope is not correct, we want to avoid the rest of the controller action from happening. there might be a better way to handle it, will take a look. if you have any suggestions, let us know.

[DavidParks8]

httpContext needs locking to avoid concurrent reads with external code.

[DavidParks8]

Using Task.Run is usually something that shouldn't be done in an already async function for perf/context switching reasons. It looks like it is only done to measure things. I suggest removing task.run and measuring with StopWatch.StartNew.

[DavidParks8]

ReaderWriterLockSlim is disposable. Thus MsalSessionTokenCacheProvider should also be disposable.

[DavidParks8]

rather than iterating the claims 4 times, it can all be done in one .Any call.