Skip to content

Add microbenchmarks for Crc32 and Crc64 algorithms#3035

Merged
cincuranet merged 3 commits intodotnet:mainfrom
brantburnett:crc-benchmarks
Nov 30, 2023
Merged

Add microbenchmarks for Crc32 and Crc64 algorithms#3035
cincuranet merged 3 commits intodotnet:mainfrom
brantburnett:crc-benchmarks

Conversation

@brantburnett
Copy link
Contributor

@brantburnett brantburnett marked this pull request as ready for review May 22, 2023 19:21
@brantburnett
Copy link
Contributor Author

@adamsitnik

Here are the CRC benchmarks per your request. I do see an execution failure in one case in CI, but my attempts to make sense of it have hit a dead end. I don't think it's related to this change, but can't be certain.

@danmoseley
Copy link
Member

Yea, it's not your change, some fragility in a winforms test.

@danmoseley danmoseley requested a review from adamsitnik May 22, 2023 20:55
danmoseley
danmoseley previously approved these changes May 22, 2023
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@brantburnett thank you for your contribution! Overall it looks good, but PTAL at my comments.

{
Crc.Append(Buffer);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for GetCurrentHash. I didn't think any of the other methods warrant benchmarks, except possibly GetCurrentHashAsUInt32 or HashToUInt32. However, both of those are only available on the .NET 8 packages.

I did use separate classes for the GetCurrentHash tests because they don't need to run on different data lengths.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@brantburnett thank you for your contribution! Overall it looks good, but PTAL at my comments.

brantburnett and others added 2 commits May 23, 2023 08:07
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>
@cincuranet
Copy link
Contributor

@adamsitnik is there anything that prevents this from merging?

@cincuranet
Copy link
Contributor

I'm going to merge this as it is now and add the remaining benchmarks myself in separate PR.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants