Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use FixedTimeEquals in NETCore targets #2857

Merged
merged 7 commits into from
Oct 10, 2024
Merged

Conversation

westin-m
Copy link
Contributor

@westin-m westin-m commented Sep 30, 2024

Closes #2825

There is another class in wilson named CryptographicOperations, so the fully qualified name is necessary.

without FixedTimeEquals Mean Error StdDev
AreEqual_ByteArrays 31.59 ns 0.185 ns 0.155 ns
AreEqual_ByteSpans 30.66 ns 0.169 ns 0.141 ns
with FixedTimeEquals Mean Error StdDev
AreEqual_ByteArrays 150.2 ns 0.47 ns 0.44 ns
AreEqual_ByteSpans 160.1 ns 0.56 ns 0.52 ns

The following results are after applying NoOptimization

without FixedTimeEquals Mean Error StdDev
AreEqual_ByteArrays 173.8 ns 0.36 ns 0.33 ns
AreEqual_ByteSpans 154.4 ns 0.21 ns 0.19 ns
with FixedTimeEquals Mean Error StdDev
AreEqual_ByteArrays 157.8 ns 1.96 ns 1.83 ns
AreEqual_ByteSpans 155.4 ns 0.20 ns 0.19 ns

@westin-m westin-m requested a review from a team as a code owner September 30, 2024 22:32
kllysng
kllysng previously approved these changes Sep 30, 2024
@pmaytak
Copy link
Contributor

pmaytak commented Sep 30, 2024

Could you also run a small benchmark?

@keegan-caruso
Copy link
Contributor

A similar change is needed for the ReadOnlySpan overload.

@armandRobled
Copy link

Patche-2_Robledo6

@kllysng
Copy link
Contributor

kllysng commented Oct 1, 2024

Could you also run a small benchmark?

Are we okay with these benchmarks? Not sure if the results are as expected 🤔

@kllysng kllysng dismissed their stale review October 1, 2024 17:51

Benchmarks

@westin-m westin-m closed this Oct 2, 2024
@westin-m westin-m reopened this Oct 8, 2024
@westin-m
Copy link
Contributor Author

westin-m commented Oct 8, 2024

Reopening this because our current code is getting optimized, which we don't want.

Copy link
Contributor

@pmaytak pmaytak left a comment

Choose a reason for hiding this comment

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

Both of these methods are covered by tests?

@westin-m
Copy link
Contributor Author

westin-m commented Oct 8, 2024

Both of these methods are covered by tests?

Yes, there is already coverage.

@westin-m westin-m merged commit a7c4b65 into dev Oct 10, 2024
6 checks passed
@westin-m westin-m deleted the westin/useFixedTimeEquals branch October 10, 2024 00:33
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.

[Feature Request] Use fixedtimeequals on Net Core targets
7 participants