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 BitOperations in couple more places #101701

Merged
merged 26 commits into from
Jun 3, 2024

Conversation

PaulusParssinen
Copy link
Contributor

@PaulusParssinen PaulusParssinen commented Apr 29, 2024

Should be files that are only included by NetCoreAppToolCurrent projects.

  • UPDATE1: Made ILVerification library consumed by ILVerify tool target NetCoreAppToolCurrent (same as target as ILVerify)
  • UPDATE2: Never mind, I didn't realize it was shipped as NuGet package.. Reverting..
answered question I want to use this to ask, whether any contributions to ``src/coreclr/tools/Common/Internal/NativeFormat/`` is welcomed as the files around here are largely untouched from their initial import and could utilize some newer APIs by my quick skim through. Should these be left alone?

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 29, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Apr 29, 2024
@am11 am11 added area-Meta and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 29, 2024
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@@ -2051,7 +2046,7 @@ private void ComputeLayout()
uint bucketsEstimate = (uint)(_Entries.Count / _nFillFactor);

// Round number of buckets up to the power of two
_nBuckets = (uint)(1 << HighestBit(bucketsEstimate));
_nBuckets = (uint)HighestBit(bucketsEstimate);
Copy link
Member

Choose a reason for hiding this comment

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

Should be better to use RoundUpToPowerOf2. The old implementation would round up 2^n to next bigger value.

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 noticed this comment yesterday too, will use that instead.. thanks!

Copy link
Contributor Author

@PaulusParssinen PaulusParssinen Apr 30, 2024

Choose a reason for hiding this comment

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

Actually, I think the comment here is inaccurate in that the existing code did not round up to the power of two but only set the highest bit.

e.g.

int bucketEstimate =                                                        0b0000_1010;
int highestBitSet = 1 << HighestBit(bucketEstimate);            // returns: 0b0000_1000
int nextPow2 = BitOperations.RoundUpToPowerOf2(bucketEstimate); // returns: 0b0001_0000

whether it should, is another question.. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, at a glance it rather looks like the logic for HighestBit is incorrect. It claims to be computing the following, where one would normally interpret "rounded up" as meaning the actual algorithm is 1 + ceil(log2(x)) for non-zero x

Returns 1 + log2(x) rounded up, 0 iff x == 0

What's actually happening, however, is that there is no rounding up and it's simply doing 1 + log2(x) for non-zero x:

Console.WriteLine($"{HighestBit(0)} {BitOperations.Log2(0)}");  // 0 0 -- correct
Console.WriteLine($"{HighestBit(1)} {BitOperations.Log2(1)}");  // 1 0 -- correct
Console.WriteLine($"{HighestBit(2)} {BitOperations.Log2(2)}");  // 2 1 -- correct
Console.WriteLine($"{HighestBit(3)} {BitOperations.Log2(3)}");  // 2 1 -- should be 3, as log2(3) is 1.58496...
Console.WriteLine($"{HighestBit(4)} {BitOperations.Log2(4)}");  // 3 2 -- correct
Console.WriteLine($"{HighestBit(5)} {BitOperations.Log2(5)}");  // 3 2 -- should be 4, as log2(5) is 2.321928...
...

@MichalStrehovsky
Copy link
Member

I want to use this to ask, whether any contributions to src/coreclr/tools/Common/Internal/NativeFormat/ is welcomed as the files around here are largely untouched from their initial import and could utilize some newer APIs by my quick skim through. Should these be left alone?

They were not touched because there were no problems with them and they are not perf hotspots. They can be changed. Note that the files with Gen suffix are generated with the https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/Common/Internal/Metadata/NativeFormat/UpdateNativeFormatSources.cmd script so updating them involves updating the generator and re-running it to generate the sources.

@PaulusParssinen
Copy link
Contributor Author

PaulusParssinen commented Apr 30, 2024

They were not touched because there were no problems with them and they are not perf hotspots. They can be changed. Note that the files with Gen suffix are generated with the https://github.com/dotnet/runtime/blob/main/src/coreclr/tools/Common/Internal/Metadata/NativeFormat/UpdateNativeFormatSources.cmd script so updating them involves updating the generator and re-running it to generate the sources.

Okay, thanks for the advice! Unrelated to this PR but should those genered files be perhaps updated to follow the .g.cs pattern? Or is the contribution bar in these parts: if it works, don't touch? 😄

@MichalStrehovsky
Copy link
Member

Okay, thanks for the advice! Unrelated to this PR but should those genered files be perhaps updated to follow the .g.cs pattern? Or is the contribution bar in these parts: if it works, don't touch? 😄

We're not very consistent with doing this (this is not the only "offline generator") but it might be an improvement to rename them to .g.cs.

@PaulusParssinen
Copy link
Contributor Author

I'm unable to build the runtime locally for unknown reasons (#101720) so I'll close this for time being..

@PaulusParssinen PaulusParssinen marked this pull request as ready for review May 10, 2024 18:04
The only consumer of this library is ILVerify, which itself is NetCoreAppToolCurrent.
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.

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit 59e8bbc into dotnet:main Jun 3, 2024
142 of 146 checks passed
@PaulusParssinen PaulusParssinen deleted the use-bitoperations-more branch June 5, 2024 14:13
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants