Use BitOperations in couple more places - #101701
#101701Use BitOperations in couple more places#101701MichalStrehovsky merged 26 commits intodotnet:maindotnet/runtime:mainfrom PaulusParssinen:use-bitoperations-morePaulusParssinen/runtime:use-bitoperations-moreCopy head branch name to clipboard
BitOperations in couple more places#101701Conversation
Big thanks to @MichalPetryka for showing me simpler alternative to this
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
|
|
||
| // Round number of buckets up to the power of two | ||
| _nBuckets = (uint)(1 << HighestBit(bucketsEstimate)); | ||
| _nBuckets = (uint)HighestBit(bucketsEstimate); |
There was a problem hiding this comment.
Should be better to use RoundUpToPowerOf2. The old implementation would round up 2^n to next bigger value.
There was a problem hiding this comment.
I noticed this comment yesterday too, will use that instead.. thanks!
There was a problem hiding this comment.
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_0000whether it should, is another question.. 🤔
There was a problem hiding this comment.
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...
...
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 |
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. |
|
I'm unable to build the runtime locally for unknown reasons (#101720) so I'll close this for time being.. |
- Yep..
The only consumer of this library is ILVerify, which itself is NetCoreAppToolCurrent.
* ILVerification was shipped OOB
This reverts commit 6049c07.
Should be files that are only included by
NetCoreAppToolCurrentprojects.NetCoreAppToolCurrent(same as target as ILVerify)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?