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

Allow conversion of EnsoBigInteger to double #3865

Merged
merged 12 commits into from
Nov 24, 2022

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Nov 10, 2022

Pull Request Description

Make sure any Enso number (including EnsoBigInteger) can be passed to Java via Truffle interop and used on the Java side.

Notes

The problem is that there is no support for java.math.BigInteger in Truffle interop (as of GraalVM 22.3). We need some "approximations". There is a special handling for everything that "looks like number" - e.g. extends java.lang.Number in the Truffle interop. Let's use that! That's the reason why EnsoBigInteger now extends Number.

That opens three possibilities on Java side:

  • loose precision as doubleArrayAverage method shows. Code is simple, and the result is so-so OK. Good for statistical functions that don't have to be precise.
  • get exact representation and then loose precision as shown by numberArrayAverage
  • get exact representation and convert to java.math.BigInteger via toString as the last test shows - slow, but precise

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Java,
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Nov 10, 2022
@JaroslavTulach JaroslavTulach self-assigned this Nov 10, 2022
@JaroslavTulach JaroslavTulach removed the CI: Ready to merge This PR is eligible for automatic merge label Nov 11, 2022
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Nov 11, 2022

Marcin has noted: "This breaks even the truffle contract"

obrazek

That's certainly a valid concern, @kustosz. Let's address it in 2bea701 - by checking the code in HostToTypeNode and HostUtil I noticed that Truffle languages may expose any java.lang.Number subclass from own language to interop. As such the 2bea701 just makes EnsoBigInteger extends java.lang.Number. That doesn't violate any Truffle contract and still allows us to get on hold of even big Numbers from the host code.

@@ -19,6 +20,32 @@ public static long add(long a, long b) {
return a + b;
}

public static double doubleArrayAverage(double[] arr) {
Copy link
Member Author

Choose a reason for hiding this comment

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

The averageOfMixedArrayOverDouble test prepares array of power of 2 with 200 elements. Some of them are Long, some of them are EnsoBigInteger.

However the Java side can request them to be converted to double - useful for example for statistic functions where the precision doesn't matter much.

return sum / arr.length;
}

public static double numberArrayAverage(Number[] arr) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should one need to distinguish between various Enso Number types, one can accept Number[]. The array would contain 63 Long numbers and the rest would be EnsoBigInteger instances that also extend java.lang.Number.

var sum = BigInteger.ZERO;
for (int i = 0; i < arr.length; i++) {
var n = arr[i] instanceof Long l ? BigInteger.valueOf(l) :
new BigInteger(arr[i].toString());
Copy link
Member Author

Choose a reason for hiding this comment

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

If one needs to get the precise value of EnsoBigInteger one can use its toString() method and parse the value back.

Alternative would be to make the number implement Callable<java.math.BigInteger> or something other JDK interface (Supplier, Function, etc.) to provide the internal instance of BigInteger.

@JaroslavTulach
Copy link
Member Author

I think the functionality is ready for your review, gentlemen! I am awaiting your comments.

@JaroslavTulach JaroslavTulach added the CI: Ready to merge This PR is eligible for automatic merge label Nov 24, 2022
Copy link
Contributor

@4e6 4e6 left a comment

Choose a reason for hiding this comment

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

Jaroslav convinced me that it is the best we can do for interop but I feel like I may not understand the implications of this change fully

@mergify mergify bot merged commit f225a96 into develop Nov 24, 2022
@mergify mergify bot deleted the wip/jtulach/ExposeBigIntegerAsDouble_182962982 branch November 24, 2022 10:00
@JaroslavTulach
Copy link
Member Author

FYI: There is going to be "official" support for BigInteger interop: oracle/graal#5490 in the future version of GraalVM.

@@ -16,7 +16,7 @@
/** Internal wrapper for a {@link BigInteger}. */
@ExportLibrary(InteropLibrary.class)
@ExportLibrary(TypesLibrary.class)
public final class EnsoBigInteger implements TruffleObject {
public final class EnsoBigInteger extends Number implements TruffleObject {
Copy link
Member Author

Choose a reason for hiding this comment

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

Got a comment from Christian Humer:

Your trick with EnsoBigInteger extends Number will not work on Espresso for example.

I guess we'll have to switch to official BigInteger interop once it is available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants