Skip to content

Commit

Permalink
[Truffle] Fixed marshaling of Floats.
Browse files Browse the repository at this point in the history
@nirvdrum
nirvdrum committed Jan 5, 2016
1 parent ba286b5 commit 66a595d
Showing 3 changed files with 11 additions and 6 deletions.
1 change: 0 additions & 1 deletion spec/truffle/tags/core/marshal/dump_tags.txt
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
fails:Marshal.dump dumps an extended_object
fails:Marshal.dump with a Float dumps a Float
fails:Marshal.dump with a String dumps a String extended with a Module
fails:Marshal.dump with a Regexp dumps a Regexp
fails:Marshal.dump with a Regexp dumps a Regexp with flags
3 changes: 0 additions & 3 deletions spec/truffle/tags/core/marshal/float_tags.txt
View file

This file was deleted.

13 changes: 11 additions & 2 deletions truffle/src/main/java/org/jruby/truffle/nodes/rubinius/FloatPrimitiveNodes.java
View file
Original file line number Diff line number Diff line change
@@ -39,19 +39,28 @@ public FloatDToAPrimitiveNode(RubyContext context, SourceSection sourceSection)
@TruffleBoundary
@Specialization
public DynamicObject dToA(double value) {
String string = String.format(Locale.ENGLISH, "%.9f", value);
// Large enough to print all digits of Float::MIN.
String string = String.format(Locale.ENGLISH, "%.1022f", value);

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Jan 6, 2016

Member

This is the base 2 exponent of Float::MIN, the base 10 is -308 (normal) or -322. And then add for the few digits, in the case of Double.MIN_VALUE I found 325.
The approach of creating substring after seems highly inefficient though, could we just get the relevant bits with something like "%.20g" ?

This comment has been minimized.

Sign in to view

Copy link
@nirvdrum

nirvdrum Jan 6, 2016

Author Contributor

I blindly bumped the value to match Rubinius and copied their comment accordingly. It was a bit hard to verify since our version of Float::MIN seems to have a different level of precision than theirs does.

I'll try the different format string. Rubinius has a much more convoluted (and efficient) method than what we're using. But this is behind a boundary and only used to marshal a Float.

This comment has been minimized.

Sign in to view

Copy link
@eregon

eregon Jan 6, 2016

Member

Yeah, it seems weird to me Float are not using their native binary representation, I thought it was portable.
Anyway, MRI also uses dtoa is seems, which goes to some expensive double-to-base10string conversion, which String.format also uses, so that's good to reuse, but while using it we might as well take advantage of its formatting abilities 😄

if (string.toLowerCase(Locale.ENGLISH).contains("e")) {
throw new UnsupportedOperationException();
}

string = string.replace("-", "");
while (string.charAt(string.length() - 1) == '0') {
string = string.substring(0, string.length() - 1);
}

final int decimal;
int decimal;

if (string.startsWith("0.")) {
string = string.replace("0.", "");
decimal = 0;

while (string.charAt(0) == '0') {
string = string.substring(1, string.length());
--decimal;
}
} else {
decimal = string.indexOf('.');

0 comments on commit 66a595d

Please sign in to comment.