Rewrote Rational.Str to speed up stringification, Rats somewhat, FatRats a lot #1807
Conversation
src/core/Rat.pm6
Outdated
| ) ~ $whole; | ||
|
|
||
| if $fract { | ||
| if $!denominator.chars < 5 { |
There was a problem hiding this comment.
Can't this be just $!denominator < 10000? That would be faster than converting it to a string to count the chars.
There was a problem hiding this comment.
Good point. I'll fix that.
… Rats. Significantly faster for large denominator FatRats; 2 to 200 times the speed of the original. Returns slightly different results. The original sometimes left trailing zeros. This always removes trailing zeros. Spectest clean.
… Rats. Slightly faster for all Rats. Significantly faster for large denominator FatRats; 2 to 200 times the speed of the original. Spectest clean.
thundergnat
changed the title
| # fight floating point noise issues RT#126016 | ||
| if $fract.Num == 1e0 { ++$whole; $fract = 0 } | ||
| if $fract.Num == 1e0 { | ||
| if nqp::unbox_s(self.^name) eq 'Rat' { |
There was a problem hiding this comment.
I think this could/should be if nqp::eqaddr(self.WHAT,Rat) {. Then you're actually checking against the type itself, not the string representation of its name.
There was a problem hiding this comment.
I am not arguing, but I did just copy that syntax from line 14 in the same file. I can certanly change that if it is a better way to go.
There was a problem hiding this comment.
Yeah, but that line is intentionally creating a string. Line 12 does nqp::eqaddr(self.WHAT,Rational), because there it's checking for a type. If you grep through the Rakudo sources you'll see a whole lot more uses of nqp::eqaddr(<something>.WHAT, <some type>) than <something>.^name eq <some type name>. What you have would work, it's just slower.
| if $fract.Num == 1e0 { | ||
| if nqp::unbox_s(self.^name) eq 'Rat' { | ||
| $whole += 1; | ||
| $fract = 0 |
There was a problem hiding this comment.
This is kind of a nitpick, but I think there's a convention that last statements in blocks only leave off the semi-colon if the value is being returned.
| $whole += 1; | ||
| $fract = 0 | ||
| } | ||
| $reduce = True |
| $fract *= 1000000; | ||
| } else { | ||
| $c = $!denominator.chars + 1; | ||
| $fract *= nqp::pow_I(10, nqp::decont($c), Num, Int); |
There was a problem hiding this comment.
The use of nqp ops seems a little inconsistent. Why use nqp::pow_I instead of just **? Or conversely, why use e.g., $!denominator.chars instead of nqp::chars($!denominator.Str) a line above?
There was a problem hiding this comment.
I did some testing both ways and the nqp::pow_I seemed to be marginally but measurably faster. I could easily change that back though. I am not really familiar with the different nqp ops and which give enough of a benefit to outweigh the added complexity.
Thanks for your review.
There was a problem hiding this comment.
What you have is definitely not wrong, so now we're getting into personal preference. But since you're changing the algorithm used (and the original code is almost completely pure Perl 6), maybe a first commit that converts to the new algorithm, and then a second commit that changes some of the new code's slow parts to nqp ops?
| # maintain precision (trailing zeros) of large denominator FatRats. | ||
| # There are situations where the extra zeros are significant, they | ||
| # can be easily removed in userspace but not easily restored | ||
| if ($c < 8) || (nqp::unbox_s(self.^name) eq 'Rat') || $reduce { |
There was a problem hiding this comment.
Same thing about nqp::eqaddr(self.WHAT,Rat) as above.
|
A nit: the cuddled else (1) is not consistent with the rest of the code and (2) ignores the style guide (see CONTRIBUTING.md). |
|
Obsolete. Rewrote following critiques and suggestions |
Moved Rational.Str to Rat.Str. Add a fast FatRat.Str multi.Rewrote Rational.Str to speed up Rational stringification across the board. Speeds up Rats marginally. Speeds up FatRats, especially those with large denominators significantly. Takes from half, to 1/200th the time that it used to take.Spectest clean.