quamina icon indicating copy to clipboard operation
quamina copied to clipboard

improvement: alternative numbits conversion

Open arnehormann opened this issue 1 year ago • 3 comments

... and drop a doc-reference to a function that was not ported from the original implementation.

The improvement will use sign extension for a right shift for int64. This will fill all bits with the sign bit. There's no need for another constant.

This was sensibly suggested by Raph Levien in a mastodon thread: https://mastodon.online/@raph/113071041069390831

arnehormann avatar Oct 10 '24 16:10 arnehormann

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.70%. Comparing base (158aeb1) to head (a85681a).

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
- Coverage   96.94%   96.70%   -0.24%     
==========================================
  Files          21       21              
  Lines        1996     2461     +465     
==========================================
+ Hits         1935     2380     +445     
- Misses         35       55      +20     
  Partials       26       26              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 10 '24 16:10 codecov-commenter

Sorry Arne, am still in the chaos of moving house, I'm sure I'll just accept this when I get free in a week or so.

timbray avatar Oct 16 '24 03:10 timbray

If it helps, the change matches what ruler has at https://github.com/aws/event-ruler/blob/main/src/main/software/amazon/event/ruler/ComparableNumber.java#L65. This code has been running without any issues in shadow mode on production workloads for a while now. I don’t understand the float -> int -> uint conversion but I imagine that’s peculiar to go number types.

baldawar avatar Oct 18 '24 23:10 baldawar

@baldawar it's something I knew and should have done, too. One of these "obvious in hindsight" things, like numbits itself: Right shifting is a division by two to the power of the number of bits. Due to twos-complement, high bits have to be filled with the first bit to make this work with signed numbers. Java has no unsigned primitive numeric types, so they provide two kinds of right shift: one that fills new high bits with 0 and one that fills the new high bits with copies of the previous highest bit. If you have a positive unsigned value with the highest bit set (greater or equal than 1<<63) or for regular binary operations, you need to use the "fill with zeroes" version (in Java: >>>). Go has both signed and unsigned types and has only one way to right shift. The casts are required to make this do the right thing in machine code.

In the code here and in Ruler, it's used to avoid another constant. Which is a teeny tiny bit faster but also looks nicer imo. Algorithmically, at least. In this case, Java definitely looks cleaner than Go due to its own special operator >> vs >>>.

arnehormann avatar Oct 20 '24 12:10 arnehormann