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

Introducing $*RAT-UPGRADE-CLASS #4299

Merged
merged 5 commits into from Jan 4, 2022
Merged

Introducing $*RAT-UPGRADE-CLASS #4299

merged 5 commits into from Jan 4, 2022

Conversation

lizmat
Copy link
Contributor

@lizmat lizmat commented Apr 4, 2021

This dynamic variable indicates the class in which an UPGRADE-RAT
method will be called whenever a Rat has a denominator that does not
fit in a 64bit native integer.

The default setting for $*RAT-UPGRADE-CLASS is Num: this will silently
downgrade a Rat to a Num (floating point), thus sacrificing precision
for speed (the current behaviour).

Other possible settings are:

  • CX::Warn downgrade to Num but warn
  • FatRat silently upgrade to FatRat
  • Failure don't upgrade, return an appropriate Failure
  • Exception don't upgrade, throw an appropriate Exception

Although the dynamic variable indicates a "class" is expected, there
are no technical obstacles to put anything else in there, as long
as it can dispatch to a "UPGRADE-RAT" method that takes two Ints:
the numerator and denominator.

This does not affect the inlineability of the CREATE_RATIONAL_FROM_INTS
subs, but it does make the default case of silently downgrading to Num
slightly slower. Currently this slowness is mostly caused by &DYNAMIC
being slow, and no locally cached dynamic variables (which will hopefully
change in the not-so-distant future).

Inspired by the discussions at:

https://www.reddit.com/r/rakulang/comments/mipog4/raku34_python19_extreme_math_steve_roe/
https://twitter.com/liztormato/status/1378294510430601220

@lizmat
Copy link
Contributor Author

lizmat commented Apr 4, 2021

See also: #3122 and Raku/problem-solving#185

@lizmat
Copy link
Contributor Author

lizmat commented Apr 4, 2021

Re Jonathan's original comment: #3122 (comment) some updated clarifications.

From a language design point of view, it's affecting the behavior of an operator, which is part of the current language, and so it should be a lexical property.

I think @jnthn and I have a fundamental difference of opinion: language architect vs module developer :-)

Either you could consider it affecting the behaviour of all operators that can potentially generate a Rat, or you could consider it changing the behaviour of a single implementation-detail sub CREATE_RATIONAL_FROM_INTS. But the latter is never exported anywhere at all.

Making it a lexical property, would disallow affecting this externally for any math libraries: if a math library wants to make sure it will never lose precision, it should use FatRats from the start. If it doesn't care, it should do its thing (which would just use Rats), and the dynamic variable will provide the necessary semantics for the user of the math library. The developer of the math library doesn't need to do anything additionally.

From a performance point of view, math code tends to be hot-path-y. Dynamic variables don't go so well with that. Furthermore, making the decision about what to do at runtime is also going to cost something.

I've confirmed that this approach does not affect inlineability of the CREATE_RATIONAL_FROM_INTS sub, and that even the upgrade path exercised with:

CREATE_RATIONAL_FROM_INTS(42,99999999999999999999999999,Rat,Rat) for ^500000

gets completely inlined (except for the call to &DYNAMIC) on repeated upgrade calls.

Furthermore, if an upgrade to FatRat is done, or a downgrade to Num, then it won't be passing this code path anymore ever. This code path only happens to Rat. Any expression that has an upgrade/downgrade will not even pass this way again, because the expression will either be using Nums or FatRats.

Copy link
Member

@vrurg vrurg left a comment

Choose a reason for hiding this comment

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

As long as performance cost is only applied when the actual up/downgrade takes place, I like it.

A lexical implementation as I see it (via a scalar local to Rat) may cause more troubles than it worth performance-wise.

@lizmat
Copy link
Contributor Author

lizmat commented Apr 5, 2021

@vrurg yes, only when actual downgrade takes place. The only real change is:

!! nqp::p6box_n(nqp::div_In(nu,de)) # downgrade to float

to:

!! $*RAT-UPGRADE-CLASS.UPGRADE-RAT(nu, de)

The rest of the PR is setting up the UPGRADE-RAT methods in the various classes.

@gfldex
Copy link
Contributor

gfldex commented Apr 5, 2021

I believe $*RAT-UPGRADE-DELEGATE would be more descriptive. Would also accepting (and calling) a Callable make sense?

@lizmat
Copy link
Contributor Author

lizmat commented Apr 6, 2021

@gfldex The current implementation will take anything on which an UPGRADE-RAT method can be called. I want to keep the logic there as small as possible, as it is a very hot code path, so anything stopping it from being inlined, would immediately visible in arithmetic performance.

As to the name: perhaps $*RAT-UPGRADE-POLICY would be better? As it does not need to have to be a class in there, but it could also be an instance of a class, or anything else that implements the CALL-ME method.

@gfldex
Copy link
Contributor

gfldex commented Apr 6, 2021

POLICY is much better then CLASS. A class doesn't really exist at runtime. It's just a declarator for type objects. Since methods can be build and added to objects at runtime, CALL-ME is dynamic enough to get the same effect then a Callable.

This dynamic variable indicates the *class* in which an UPGRADE-RAT
method will be called whenever a Rat has a denominator that does not
fit in a 64bit native integer.

The default setting for $*RAT-UPGRADE-CLASS is Num: this will silently
downgrade a Rat to a Num (floating point), thus sacrificing precision
for speed (the current behaviour).

Other possible settings are:

- CX::Warn    downgrade to Num but warn
- FatRat      silently upgrade to FatRat
- Failure     don't upgrade, return an appropriate Failure
- Exception   don't upgrade, throw an appropriate Exception

Although the dynamic variable indicates a "class" is expected, there
are no technical obstacles to put anything else in there, as long
as it can dispatch to a "UPGRADE-RAT" method that takes two Ints:
the numerator and denominator.

This does *not* affect the inlineability of the CREATE_RATIONAL_FROM_INTS
subs, but it does make the default case of silently downgrading to Num
slightly slower.  Currently this slowness is mostly caused by &DYNAMIC
being slow, and no locally cached dynamic variables (which will hopefully
change in the not-so-distant future).

Inspired by the discussions at:

  https://www.reddit.com/r/rakulang/comments/mipog4/raku34_python19_extreme_math_steve_roe/
  https://twitter.com/liztormato/status/1378294510430601220
src/core.c/Rat.pm6 Outdated Show resolved Hide resolved
@raiph
Copy link
Contributor

raiph commented Apr 9, 2021

Just some bikeshedding, but...

It seems odd that an "UPGRADE" could be a fatal exception or a warning message or a conversion to a float. And "POLICY" seems redundant.

Why not $*RAT-OVERFLOW?

@@ -83,6 +93,7 @@ multi sub CREATE_RATIONAL_FROM_INTS(Int:D $nu, Int:D $de, Any, Any) is raw {
nqp::p6bindattrinvres(nqp::create(Rat),Rat,'$!numerator',$nu),
Rat,'$!denominator',$de
)
!! $*RAT-UPGRADE-POLICY.UPGRADE-RAT(nu, de)
!! nqp::p6box_n(nqp::div_In($nu,$de)) # downgrade to float
Copy link
Contributor

Choose a reason for hiding this comment

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

Syntax error here, looks like you meant to replace the nqp::p6box_n(nqp::div_In($nu,$de)) # downgrade to float with $*RAT-UPGRADE-POLICY.UPGRADE-RAT(nu, de), but accidentally just copied it in.

src/core.c/Rat.pm6 Outdated Show resolved Hide resolved
@lizmat lizmat merged commit f737b08 into master Jan 4, 2022
@lizmat lizmat deleted the rat-upgrade-class branch January 4, 2022 12:03
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.

None yet

5 participants