This was odd, nothing in the RFC says this is valid.
Should we check that there is only one value?

You're now using the implicit string converter that does the same thing here.

A multi-value header will fail the normal parse if there are multiple values anyways, don't worry about it.

Is it allocating right now even for a single value?

Is it allocating right now even for a single value?

No.

What's the return value if temp is too small?

false

You're now using the implicit string converter that does the same thing here.

A multi-value header will fail the normal parse if there are multiple values anyways, don't worry about it.

Is this the right behavior? a 500?

Probably. We don't know why it would fail and would have to debug it when it did.

This should be a 400 but we can wait until it happens 😄

@blowdart Is caching SHA1 ok? I know it's reusable, I'm just wondering if the OS resource usage is ok to keep around.

So this ends up allocating one for every threadpool thread and they live for the lifetime of the app? That's not great.

@bartonjs for his thoughts

SHA-1 state is 96 bytes. Even if you assume a couple of layers of indirection and metadata it shouldn't be all that large.

The other main alternative would be a static ConcurrentBag for pooling (or going back to a new one per request).

We already pool to ton of stuff like this and the savings seem significant.

Pooling this gave a 2x gain, so the layers of indirection seem pretty heavy.

What do we want to do here? Go back to the slow path and create a new SHA1 every request, keep the ThreadStatic, or maintain a small pool using something like ConcurrentBag and creating a temporary new one if the pool is empty?

At 24 + 36 bytes I don't know if it would be faster to use IncrementalHash with two calls to AppendData or to do the copy (probably doing this copy).

You might see perf improvement from using IncrementalHash anyways, if you happen to be at a level where you can notice a 3us difference.

I'll check this out

Tried it out and it looked slower

Is there any chance for an exception left in this code-path?

Convert.TryFromBase64String throws if value-argument is null, but this case is already eliminated in L98 here.
So you could remove the try-catch, resulting in smaller code.

Actually I would write this method as

public static bool IsRequestKeyValid(string value)
{
    ReadOnlySpan<char> chars = value.AsSpan();

    if (chars.IsEmpty)
    {
        return false;
    }

    Span<byte> temp = stackalloc byte[16];
    var success = Convert.TryFromBase64Chars(chars, temp, out int written);

    // RyuJIT produces better code than the other way round: success && written == 16
    return written == 16 && success;
}

Maybe: to get even more perf, one could skip the base64-decoding step, and just check if the chars.Length == 16 and if the chars are in the base64-alphabet, so no need to perform the actual decoding. This step could also be vectorized. But maybe that's a bit of over-engineering.

If written == 16, so for base64 the input-length must be 24. Hence one could write

ReadOnlySpan<char> chars = value.AsSpan();

if (chars.Length != 24)
{
    return false;
}

So the entire decoding-step is omitted if the result (written) can't be 16.

Maybe: to get even more perf, one could skip the base64-decoding step, and just check if the chars.Length == 16 and if the chars are in the base64-alphabet, so no need to perform the actual decoding.

I'm assuming you meant chars.Length == 24. Can't do this because a 17 and 18 byte string will also be 24 bytes when encoded. Unless you also want to check for the trailing "==", but I think at this point we're trying too hard.

I'm going to stick with Convert.TryFromBase64Chars for now, but I'll look at the other feedback :)

I'm assuming you meant chars.Length == 24

Yep, typed wrong in the first comment.

Can't do this because a 17 and 18 byte string will also be 24 bytes when encoded.

Maybe I missunderstand something now, but the string is encoded and we try to decode it, then check if the decoded data (byte) is of size 16.
So the other way round this means, 16 data bytes -> 24 base64 encoded utf8-bytes.
And for base64 the encoded length is always a multiple of 4.

Thus if chars.Length != 24 we know that the decoded data won't be 16 bytes, so we can short-circuit the method and return false. No need to try to decode the input.

If chars.Length == 24 we can try to decode, then check if written == 16.

Ahhhhh, now I see that in my previous comment it may look like the entire method. It's just a fragment. Sorry.

So I mean:

public static bool IsRequestKeyValid(string value)
{
    ReadOnlySpan<char> chars = value.AsSpan();

    if (chars.Length != 24)
    {
        return false;
    }

    Span<byte> temp = stackalloc byte[16];
    var success = Convert.TryFromBase64Chars(chars, temp, out int written);

    return written == 16 && success;
}

If you want I could submit a PR for the vectorized version (next week).

Move these exceptions out of the hot-path into ThrowHelper-methods?

What if this is called with a requestKey that isn't 24 bytes? Should we check the return value of GetBytes?

"requestKey is already verified to be small (24 bytes) by 'IsRequestKeyValid()' and everything is 1:1 mapping to UTF8 bytes"