Implement Number.round as a builtin

[GregoryTravis]

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

• The documentation has been updated, if necessary.
• Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
• All code follows the
  Scala,
  Java,
  and
  Rust
  style guides. In case you are using a language not listed above, follow the Rust style guide.
• All code has been tested:
  • Unit tests have been written where possible.
  • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

[JaroslavTulach]

Few suggestions to make the code more ready for partial evaluation (PE). However I am not sure applying my advises will be enough. We have to use IGV to investigate the compiler internals. Please rewrite the benchmarking code to be ready for such analysis.

[JaroslavTulach]

I am getting a NPE:

sbt:std-benchmarks> benchOnly integers_new_round_decimal_places0_use_bankersTrue
[info] java.lang.NullPointerException: Cannot invoke "org.enso.benchmarks.BenchGroup.specs()" because "group" is null
[info]  at org.enso.benchmarks.Utils.findSpecByName(Utils.java:47)
[info]  at org.enso.benchmarks.generated.Numbers.setup(Numbers.java:93)
[info]  at org.enso.benchmarks.generated.jmh_generated.Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest._jmh_tryInit_f_numbers0_G(Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest.java:434)
[info]  at org.enso.benchmarks.generated.jmh_generated.Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest.integers_new_round_decimal_places0_use_bankersTrue_AverageTime(Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest.java:161)
[info]  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

CCing @Akirathan.

[Akirathan]

I am getting a NPE:

sbt:std-benchmarks> benchOnly integers_new_round_decimal_places0_use_bankersTrue
[info] java.lang.NullPointerException: Cannot invoke "org.enso.benchmarks.BenchGroup.specs()" because "group" is null
[info]  at org.enso.benchmarks.Utils.findSpecByName(Utils.java:47)
[info]  at org.enso.benchmarks.generated.Numbers.setup(Numbers.java:93)
[info]  at org.enso.benchmarks.generated.jmh_generated.Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest._jmh_tryInit_f_numbers0_G(Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest.java:434)
[info]  at org.enso.benchmarks.generated.jmh_generated.Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest.integers_new_round_decimal_places0_use_bankersTrue_AverageTime(Numbers_integers_new_round_decimal_places0_use_bankersTrue_jmhTest.java:161)
[info]  at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

CCing @Akirathan.

Yes. With @GregoryTravis, we established that the branch https://github.com/enso-org/enso/tree/numeric-bench-jmh is the one where the benchmarks on Numeric.enso work. Should I cherry-pick commits on that branch here as well?
The PR is #7513

[GregoryTravis]

@Akirathan No need, it looks like converting to a builtin isn't going to be enough of an improvement so I'm going to close this.

[JaroslavTulach]

I solved the NPE - had some debris on my disk: With 665786c I can run for example:

sbt:std-benchmarks> benchOnly decimal_new_round_decimal_places0_use_bankersTrue

[JaroslavTulach]

I found a serious problem in the previous implementation, @GregoryTravis: Have to use PrimitiveValueProfile when profiling primitive values: dc9ebe0 Then I also fixed the throwing: efeb2d7 Now, when I run:

sbt:enso> runEngineDistribution --run test/Benchmarks/src/Numeric.enso

I believe the builtin is always faster.

[JaroslavTulach]

After 94dac30 I get the logic down to something like this:

That looks simple enough - almost everything is a constant (C(...)) and the amount of computations (with n) is limited to (almost) minimum. Right now I don't have any other ideas for improvements. @GregoryTravis please review current state and feel free to close this PR again if the results aren't good enough.

[radeusgd]

It's really interesting that there is little change for Integer, and both here and in Table. Did you check what the result vectors in these benchmarks look like? Do they surely compute the right thing?

Code looks ok, but definitely needs cleanup before merging (commented out code, unnecessary imports like IO in Number), we should probably remove round_old in the version that is going to be merged - we could alternatively move this implementation into the Benchmark itself if we want to keep a comparison - but cannot keep it in stdlib.

[JaroslavTulach]

Benchmarks after Jaroslav's last changes: definite improvement for decimal

+1

but still mysteriously worse for integers.

I've added some more changes for integers. In general it is hard to be faster for decimal places 0 and decimal places 2 when the operation is no-op! With 843f8ef one can execute:

sbt:std-benchmarks> benchOnly Numbers.integer.*round_decimal_places0_use_bankersTrue
[info] Numbers.integer_new_round_decimal_places0_use_bankersTrue  avgt    5  0.776 ± 0.028  ms/op
[info] Numbers.integer_old_round_decimal_places0_use_bankersTrue  avgt    5  0.803 ± 0.041  ms/op

e.g. the builtin and host Java interop operations are on par in this trivial case. The _decimal_places2 cases are also trivial, so the result will be the same. However _decimal_places_2 (e.g. -2) shows double speed for the builtin:

sbt:std-benchmarks> benchOnly Numbers.integer.*round_decimal_places_2_use_bankersTrue
[info] Numbers.integer_new_round_decimal_places_2_use_bankersTrue  avgt    5  1.742 ± 0.002  ms/op
[info] Numbers.integer_old_round_decimal_places_2_use_bankersTrue  avgt    5  4.088 ± 1.095  ms/op

[GregoryTravis]

My apologies, I hadn't merged properly and didn't have all of @JaroslavTulach 's changes. The benchmarks look good in both cases.
I updated the error handling and added more tests for bigintegers as well.

[radeusgd]

Do we have a test for what happens if decimal_places is not an Integer, or if use_bankers is not Boolean? If not, please add one just to be sure it is handled correctly - it should throw a Type_Error panic.

[GregoryTravis]

@JaroslavTulach @radeusgd I always keep the old implementation around for benchmarking until everything else is done and all review is finished, because bringing it back in repeatedly takes a lot of time, when there are unexpected changes. It seems to me that benchmarks are more reliable when they are run in the same execution, rather than running them separately at different branches -- I see more consistency in the comparison between the two implementations.

[GregoryTravis]

Added test for type errors.