Skip to content

Conversation

@arontsang
Copy link

No description provided.

@arontsang arontsang force-pushed the feature/fast-wildcard-certificate-lookup branch from 51b25cd to 3efadeb Compare April 4, 2025 09:00
@arontsang arontsang force-pushed the feature/fast-wildcard-certificate-lookup branch from 6945cb9 to 5540ac3 Compare April 5, 2025 10:15
@arontsang arontsang force-pushed the feature/fast-wildcard-certificate-lookup branch from 5540ac3 to d979058 Compare April 5, 2025 10:16
Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: there are quite a few blank lines that could be removed too.

Comment on lines +107 to +108
var charA = x[^i] & 0x5F;
var charB = y[^i] & 0x5F;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You compare it backwards?
Is it better then, to write the loop backwards and have normal index access?

Comment on lines +103 to +105
var length = Math.Min(x.Length, y.Length);

for (var i = 1; i <= length; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the JIT can't elide the bound-check for indexing into the spans.
I think it's better to slice the spans to the common (= min) length and use one span in the loop, in order to elide at least one bound check (the JIT should handle the downward loop (see other comment) in a recent version).

Roughly:

Suggested change
var length = Math.Min(x.Length, y.Length);
for (var i = 1; i <= length; i++)
if (x.Length < y.Length)
{
y = y.Slice(0, x.Length);
}
else if (y.Length < x.Length)
{
x = x.Slice(0, y.Length);
}
for (var i = x.Length; i >= 1; --i)

_hasBeenUpdated = true;
}

public X509Certificate2 GetCertificate(ConnectionContext connectionContext, string domainName)
Copy link

@KLuuKer KLuuKer Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants