Skip to content

Conversation

@YegorStepanov
Copy link
Contributor

@YegorStepanov YegorStepanov commented Oct 28, 2022

This simplifies the code, fixes 1 bug and improves performance.

If an additional dependency has any disadvantages, we can easily copy its files. This dependency consists of 2 files: HashCode and BitOperations (2 one-line methods)

Comment on lines 35 to 37
<PackageReference Include="Microsoft.Bcl.HashCode" Version="1.1.1">
<PrivateAssets>all</PrivateAssets>
</PackageReference>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documentation is sparse.

As far as I understood, this library will be placed in BenchmarkDotNet.dll. If the user adds Microsoft.Bcl.HashCode (same or different version) to their application, it will be a separate dll that will not override BDN.HashCode.
I'm right?

Copy link
Collaborator

@timcassell timcassell Oct 28, 2022

Choose a reason for hiding this comment

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

I'm pretty sure it will not be baked into BenchmarkDotNet.dll, but it will be a separate Microsoft.Bcl.HashCode dll added to the nuget package alongside BenchmarkDotNet.dll, rather than a nuget reference. I'm not sure if users get access to that library that way, I haven't actually used that option before.

What are you trying to accomplish with this? As far as I know, hashcodes should not depend on an implementation, as long as they do not change during the process, so it shouldn't matter if users have a different version of the library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If myapp doesn't add own HashCode package, Microsoft.Bcl.HashCode.dll is missing when I publish myapp with modified BDN, so I think it is inside BenchmarkDotNet.dll.

But when myapp adds HashCode package, the dll appears.

Problem

It looks that they use the same version:

MyApp.dll

image

BenchmarkDotNet.dll

image

I want to disable package sharing:

For example: BDN uses HashCode v1.1.1, if the user has HashCode v1.0.0, BDN should still use v1.1.1, but the user code will use its own version.
As far as I understand, currently the largest version will be used for BDN and user code. This's bad, because the user can use v1.0.0, which is slower than v1.1.1, but BDN will bump the version.

If I understand correctly how it works...

Copy link
Collaborator

@timcassell timcassell Oct 28, 2022

Choose a reason for hiding this comment

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

If the worry is giving false benchmarks results due to nuget package versions, then shouldn't all packages be made private (except ones that are required for public APIs)? I don't think that's a concern specific to Microsoft.Bcl.HashCode.

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 don't know msbuild very well, maybe it doesn't work like that at all. I think someone would send an issue about this.

ILSpy

ILSpy does not show the nuget version too. It always v1.0.0.0 for some reason.

Perhaps all versions of Microsoft.Bcl.HashCode have the same code and decompilers show the hash code of the source.

Maybe those weird numbers show the unique package order? 🤔

image

Copy link
Member

Choose a reason for hiding this comment

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

Whenever it's possible, we try to avoid having extra dependencies. Sometimes, we have strong reasons to reference a package. For example, we reference the Iced library in order to have an embedded disassembler since it would be too hard to implement it from scratch and maintain the implementation.

In the case of Microsoft.Bcl.HashCode, I do not see any strong reasons to introduce a dependency. The current HashCode implementations work just fine. Thus, I'm strongly against having this dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem.

Should I just fix bugs in GetHashCode() or should I add a simple HashCode class to incapsulate "math magic", low code, unchecked context etc?

Out of interest:

Is it possible to add the dependency as I wanted? Just copy the files from the PackageReference library to BDN.dll so that there are no "overrides"/"dependency resolution"?

Just copy files from Microsoft.Bcl.HashCode

I tried, but unfortunately a large internal BCrypt class is required for HashCode.GenerateGlobalSeed. Perhaps we could choose seed=0, but this is too dangerous.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's OK to have a simple HashCode class. However, I would avoid copying Microsoft.Bcl.HashCode. It's enough to have a simple straightforward HashCode implementation (the performance of this method is not important).

@YegorStepanov
Copy link
Contributor Author

I made the same API as System.HashCode (except HashCode.AddBytes(ReadOnlySpan<byte>) instance method).

Added IEquatable to the Argument.

Removed last empty lines, almost all files in the solution don't have it. It will be easier to enable error if the user added an empty line.

@AndreyAkinshin AndreyAkinshin merged commit 28a8e78 into dotnet:master Nov 3, 2022
@AndreyAkinshin
Copy link
Member

@YegorStepanov thanks!

@AndreyAkinshin AndreyAkinshin added this to the v0.13.3 milestone Nov 3, 2022
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