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

Sanity check signs of raw number before comparison #125

Merged
merged 1 commit into from
Jun 22, 2022

Conversation

jedevc
Copy link
Contributor

@jedevc jedevc commented Jun 22, 2022

This fixes a performance issue when comparing large numbers to zero (or large positive numbers to large negative numbers). Without this check, the numbers are both converted to text, and string compared, which is costly if the numbers are large.

This patch provides a cheap early-exit option if the signs don't match, speeding up the cost of comparison to zero. As part of this, the .Modulo function calling .RawEquals here should also have significantly improved performance (which was otherwise an expensive check for large numbers).

This fixes a performance issue when comparing large numbers to zero (or
large positive numbers to large negative numbers). Without this check,
the numbers are both converted to text, and string compared, which is
costly if the numbers are large.

This patch provides a cheap early-exit option if the signs don't match,
speeding up the cost of comparison to zero. As part of this, the .Modulo
function calling .RawEquals should also have significantly improved
performance (which was otherwise an expensive check for large numbers).
@apparentlymart
Copy link
Collaborator

Thanks for this, @jedevc!

While the number implementation here is not intended to prioritize performance, what you proposed here does seem like a great tradeoff with relatively low additional complexity (we were already making some similar special cases here anyway) and presumably quite a significant win since it'll now just be comparing some "normal-sized" numeric values inside the Float object.

One question I had after revisiting the context around this code is that the case below makes some extra effort to avoid treating negative zero and positive zero as different and I wasn't sure if Sign would give the same treatment, but thankfully I was reassured by the Sign documentation which I hadn't previously reviewed in detail:

Sign returns:

-1 if x <   0
 0 if x is ±0
+1 if x >   0

With that concern allayed, I'm going to merge this now. Thanks again!

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.

2 participants