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

Reintroduce FNV hashing #9721

Merged
merged 3 commits into from
Feb 16, 2024
Merged

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Feb 8, 2024

Fixes: #9519, #7131

Context

Contains changes from:

Usage

<!-- Works unchanged. Identical to [MSBuild]::StableStringHash($x, 'Legacy') -->
[MSBuild]::StableStringHash($x)

<!-- 
  $hashAlgo will currently allow:
    'Legacy' - the legacy behavior (mimicking string.GetHashCode)
    'Fnv1a32bit' - Fawler-Noll-Vo 1a 32bit
    'Fnv1a32bitFast' - Custom, faster, Fawler-Noll-Vo 1a 32bit
    'Fnv1a64bit' - Fawler-Noll-Vo 1a 64bit
    'Fnv1a64bitFast' -  Custom, faster, Fawler-Noll-Vo 1a 64bit
    'Sha256' - hex string of the Sha256 hash of the given string
-->
[MSBuild]::StableStringHash($x, $hashAlgo)

Testing

  • Existing test on colissions extended for all overloads
  • Added test on expected output types

Documentation

Once PR content is agreed I'll create Doc bug + PR to update https://learn.microsoft.com/en-us/visualstudio/msbuild/property-functions?view=vs-2022#msbuild-stablestringhash
We might not possibly document all the supported algos as of now.

@JanKrivanek JanKrivanek marked this pull request as ready for review February 8, 2024 13:43
@JanKrivanek
Copy link
Member Author

FYI @KirillOsenkov - this is using the faster versions of FNV from MSBuildStructuredLog (the source and your meassurements are linked in the code)

Copy link
Member

@AR-May AR-May left a comment

Choose a reason for hiding this comment

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

LGTM.
Should we document this new feature somewhere?

Copy link
Contributor

@f-alizada f-alizada left a comment

Choose a reason for hiding this comment

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

LGTM!
Asking out of curiocity: Is there anything additionally that needs to be done in binarylog viewer after this is merged?

@JanKrivanek
Copy link
Member Author

LGTM! Asking out of curiocity: Is there anything additionally that needs to be done in binarylog viewer after this is merged?

Good question.
As a rule of thumb - Viewer needs changes when there is an addition of new derivative of BuildEventArgs or a change in definition of any existing derivatives of BuildEventArgs.
In this case we are adding property function overloads - this will not have any impact on BuildEventArgs

@JanKrivanek JanKrivanek merged commit 23f7752 into main Feb 16, 2024
8 checks passed
@JanKrivanek JanKrivanek deleted the revert-9520-revert-stablestringhash branch February 16, 2024 11:27
JanKrivanek added a commit that referenced this pull request Feb 21, 2024
JanKrivanek added a commit that referenced this pull request Feb 22, 2024
maridematte pushed a commit to maridematte/msbuild that referenced this pull request Feb 26, 2024
@JanKrivanek JanKrivanek restored the revert-9520-revert-stablestringhash branch March 13, 2024 12:23
@JanKrivanek JanKrivanek deleted the revert-9520-revert-stablestringhash branch March 13, 2024 12:47
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.

Introduce [MSBuild]::StableStringHash overload(s) with alternative hashing
3 participants