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

Add System.IO.Hashing dependency to windowsdesktop transport pack #103695

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

ViktorHofer
Copy link
Member

System.IO.Hashing is a dependency of System.Formats.Nrbf.

Didn't realize that until just now. Continuation of 91f1481

System.IO.Hashing is a dependency of System.Formats.Nrbf.

Didn't realize that until just now. Continuation of 91f1481
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

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

I'd consider bypassing the CI and just slamming it in and canceling whatever official build is in progress so that we can finish the flow today. Official build is going to be 4 hours.

@ViktorHofer
Copy link
Member Author

/ba-g this is a safe change (I double checked it) and we need this flowing asap for P6.

@ViktorHofer ViktorHofer merged commit 153296b into main Jun 19, 2024
11 of 73 checks passed
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-2 branch June 19, 2024 11:16
@ViktorHofer
Copy link
Member Author

PR and official builds taking 4 hours is just crazy, isn't it? We should bring this up and invest in reducing this to at least half.

@MichalStrehovsky
Copy link
Member

PR and official builds taking 4 hours is just crazy, isn't it? We should bring this up and invest in reducing this to at least half.

Yep. I canceled the official one that was in progress. It was only 20 minutes in, so it would take solid 8 hours to get a good build of dotnet/runtime out. Now we should have one in 4.

@am11
Copy link
Member

am11 commented Jun 19, 2024

Maybe official build should use a better pool with 32-64 cores machines? Linux distros builders, e.g., use those kind of beefy machines for official builds. Stands to reason company which owns Azure cloud can handle a pool of dozen such machines for .NET. 😉

@ViktorHofer
Copy link
Member Author

Agree on that. But the other angle is that we can build runtime so much faster than what the official build does today. In the current version of the VMR (without join steps) runtime builds in less than 30 minutes. I expect that this time will go up when we add join point legs and signing but the entire VMR will still be faster than the current runtime official build.

@MichalStrehovsky
Copy link
Member

https://dnceng.visualstudio.com/internal/_build/results?buildId=2477368&view=results

The PGO legs died on nuget restore issues. Anyone know if there's a way to rerun it while the build is still in progress? Or wait until it fails and then click rerun on the button that magically appears? Or just scrap the whole run?

@stephentoub
Copy link
Member

System.IO.Hashing is a dependency of System.Formats.Nrbf.

Does it need to be? @adamsitnik, I see System.IO.Hashing being used in one place:

    public override int GetHashCode()
    {
        int id = _id;
#if NET
        Span<int> integers = new(ref id);
#else
        Span<int> integers = stackalloc int[1] { id };
#endif
        return (int)XxHash32.HashToUInt32(MemoryMarshal.AsBytes(integers));
    }

Why does this need to use XxHash32 rather than just being:

    public override int GetHashCode() => _id;

?

stephentoub added a commit to stephentoub/runtime that referenced this pull request Jun 19, 2024
stephentoub added a commit that referenced this pull request Jun 19, 2024
* Revert "Add System.IO.Hashing dependency to windowsdesktop transport pack (#103695)"

This reverts commit 153296b.

* Remove System.IO.Hashing dependency from Nrbf

* Use HashCode
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.

None yet

4 participants