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

Adopting GraalVM's support for BigInteger #7420

Merged
merged 29 commits into from
Aug 3, 2023
Merged

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jul 28, 2023

Pull Request Description

Fixes #7213 by reacting to new isBigInteger and asBigInteger messages. Adjusts tests to require BigInteger when appropriate. EnsoBigInteger no longer extends Number. Enso tests to come.

Important Notes

Checklist

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

  • The documentation has been updated, if necessary.
  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach
Copy link
Member Author

Currently there is 40 errors like this one where the precision of a number changes.

@JaroslavTulach
Copy link
Member Author

Alas, GraalPython currently doesn't support BigIntegers:

diff --git test/Tests/src/Data/Numbers_Spec.enso test/Tests/src/Data/Numbers_Spec.enso
index 592bc7ac99..7b4879de67 100644
--- test/Tests/src/Data/Numbers_Spec.enso
+++ test/Tests/src/Data/Numbers_Spec.enso
@@ -835,6 +835,8 @@ spec =
         bigint_spec "Enso" (x->x) (*)
         bigint_spec "Java" to_java_bigint java_bigint_mul
         bigint_spec "JavaScript" to_js_bigint js_bigint_mul
+        if Polyglot.is_language_installed "python" then
+            bigint_spec "Python" to_python_bigint python_bigint_mul
 
 main = Test_Suite.run_main spec
 
@@ -844,3 +846,9 @@ foreign js to_js_bigint n = """
 
 foreign js js_bigint_mul a b = """
     return BigInt(a) * BigInt(b)
+
+foreign python to_python_bigint n = """
+    return int(n)
+
+foreign python python_bigint_mul a b = """
+    return int(a) * int(b)

fails. With oracle/graalpython#346 the Python test seems to pass OK.

CHANGELOG.md Outdated Show resolved Hide resolved
@JaroslavTulach
Copy link
Member Author

Engine (linux) and also Windows runs are fine. Time to address the change requests, fix minor issues and integrate. Engine code owners, please review.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Aug 2, 2023

Thank you for your reviews. Let's execute benchmarks and if they are OKeyish, integrate. There is also second run and 3rd run to see the trends.

Most of the Results seem fine. There is small slowdown in equalsString:

equalsString

it'd be acceptable. However there is more than 10% slowdown in caseBench3 and it will need some investigation:

caseBench3

smaller slowdowns are also visible in caseBench6, ifBench3. Some VectorBenchmarks_averageOver benchmarks are also affected - probably result of the previous case and if slowdown.

On a positive side. The StringBenchmarks_lengthOfStrings got 3% faster.

Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Can't wait to get this in and being able to move forward with stuff like #7354

@JaroslavTulach
Copy link
Member Author

Re. bench_case3 benchmark. There seems to be a difference in HostValueToEnsoNode.fallbackGuard.

fallback guard

The difference adds one additional if check which may be the reason for the slowdown. On the other hand, it is all a bit strange. Why shall we use HostValueToEnsoNode at all? It is used from Vector:

    fold : Any -> (Any -> Any -> Any) -> Any
    fold self init function =
        f = acc -> ix -> function acc (self.at ix)
        0.up_to self.length . fold init f

and all the values are coming from Enso. Very likely this is caused by defensive check - we can certainly do better if we know the Vector contains only Enso values. Let's leave this for

@JaroslavTulach JaroslavTulach added the CI: Keep up to date Automatically update this PR to the latest develop. label Aug 3, 2023
@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Aug 3, 2023
@JaroslavTulach
Copy link
Member Author

Can't wait to get this in and being able to move forward with stuff like #7354

Let's get this in. Benchmark slowdowns will be investigated later - especially in #6299.

@mergify mergify bot merged commit 9e9cf0b into develop Aug 3, 2023
23 of 24 checks passed
@mergify mergify bot deleted the wip/jtulach/BigInt_7213 branch August 3, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Keep up to date Automatically update this PR to the latest develop. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EnsoBigInteger should be a BigInteger polyglot type
6 participants