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

float format-ing regression in 9.2 #5556

Closed
kares opened this Issue Jan 13, 2019 · 7 comments

Comments

Projects
None yet
2 participants
@kares
Copy link
Member

kares commented Jan 13, 2019

ruby -e 'n = 97.65625; puts format("%f %f %.1f", n, n.round(1), n)'

Ruby 2.3, 2.5 and JRuby 9.1.17.0

97.656250 97.700000 97.7

JRuby 9.2.5.0

97.656250 97.700000 97.6

... discovered at image_optim's: toy/image_optim#168

@lassiter

This comment has been minimized.

Copy link
Contributor

lassiter commented Jan 15, 2019

Hi, @kares. Can I take this issue? I'd love to have the opportunity to contribute.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jan 15, 2019

yes please - that would be great if you took a detailed look.
if you start around RubyKernel.format and follow around where it leads to ... also if you get stuck bisecting should point you to a commit that regressed.

@lassiter

This comment has been minimized.

Copy link
Contributor

lassiter commented Jan 18, 2019

So, I think I know which method (rubySprintfToBuffer in Sprintf.java) is the cause of the error but it's unclear on exactly where inside is wrong. I finally got Eclipse running with the debugger and will know more soonish. I've got a test written so I have something to run against in the unit test folder.

Java is brand new to me, so it's a little to getting used too. This is my first time working with the language.

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jan 18, 2019

well done, yeah it might be hard to tell from all the commits
... maybe try disabling 'new' parts that seem suspicious otherwise bisecting is your friend.
alternatively, since you're suspecting, a change in Sprintf.java ... try confirming that by replacing it with code from 9.1.17's version, where possible ...

@lassiter

This comment has been minimized.

Copy link
Contributor

lassiter commented Jan 18, 2019

I've tried that but then I get a compile error. So far using bisecting, I've gotten it down to 9 commits based on my assumption but these are the ones I'm interested in most. Also, while bisecting, I get a lot of compile errors when I run mvn -Pbootstrap to run the current state against my test (second code block); So, I'm not sure exactly how to test those changes. Any ideas?

It's also difficult to be certain I know what I'm really looking for, the variables passed in Eclipse is well... a lot to go through for each step.

commit 49a74b95fbd7a00a22cdeb0a1a25192c196ecd55
Author: Miguel Landaeta <miguel@miguel.cc>
Date:   Sun Mar 25 14:26:50 2018 +0100

    Kernel#sprintf must raise ArgumentError on dangling '%' chars in format string since Ruby 2.5

commit 6a543f192ff9f770b37b23eb0c61ba2b67cede5d
Author: Miguel Landaeta <miguel@miguel.cc>
Date:   Fri Jan 5 16:37:49 2018 +0000

    Implement KeyError#receiver and KeyError#key

    For more information, please see feature #12063.

commit 056ec1ed7fdbd6054438fa0089899e8715b4d2dd
Author: kares <self@kares.org>
Date:   Tue Aug 8 11:47:10 2017 +0200

    new_float checks Float + compare bits directly instead of alloc.floats

commit 68ea9a33bf51e475ba4aab47cf3c7dfefba0409f (HEAD)
Author: kares <self@kares.org>
Date:   Tue Aug 8 11:12:24 2017 +0200

    depreacate and cleanup/simplify RubyKernel.new_float19 usage

commit bc2ba39ed88096f0d3557231e10bb22a84ee9508
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Mon Jul 31 10:00:38 2017 -0500

    [2.4] sprintf now rounds half to even.

    See https://bugs.ruby-lang.org/issues/12889

commit 472599c2c29dcb8c8d4abe1b4a932bbbb3b00671
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Wed Jul 12 17:44:40 2017 -0500

    Additional tweaks and re-ports for Float#round and friends.

commit b3c5bad9e1cecf18c586dfe9bb4dd5108de8d516
Merge: 4633080753 3a91f9e7d3
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Thu Jul 6 15:45:12 2017 -0500

    Merge branch 'master' into ruby-2.4

commit 5e779b50b9dd72726e8ccc82d855c100bac5b4e2
Author: kares <self@kares.org>
Date:   Wed Jun 28 16:34:14 2017 +0200

    fix warning on var init; prefer local runtime var; avoid stringToInum19

commit 39155ec2a841bc9d4cbc46bc9b04b45df0d3b919
Merge: cd0b9db083 fb4276238b
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Tue Mar 14 16:10:42 2017 -0500

    Merge remote-tracking branch 'origin/master' into ruby-2.4

commit b3b5d1172b92788c2c8beebcfc9e619cd98df17f
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Mon Nov 21 18:49:54 2016 -0600

    Test physical type before casting blindly.

commit a103596f667cfb5158568b6c96ec5bb43e53924b
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Thu Nov 17 09:13:29 2016 -0600

    Eliminate expectation of FIXNUM or BIGNUM from class index.

commit 4dd37024b366c088fc79400fbe1a6142529305d9
Author: Charles Oliver Nutter <headius@headius.com>
Date:   Thu Nov 17 00:53:26 2016 -0600

    Start eliminating Fixnum and Bignum in favor of Integer. #4293
require 'test/unit'

class TestFormat < Test::Unit::TestCase
  # Test Case based on https://github.com/jruby/jruby/issues/5556
  def test_sprintf_one
    n = 97.65625
    s = format("%f %f %.1f", n, n.round(1), n).to_s
    assert(s === "97.656250 97.700000 97.7", "Test failed, value of #{s} did not match '97.656250 97.700000 97.7'.")
  end
end
@lassiter

This comment has been minimized.

Copy link
Contributor

lassiter commented Jan 21, 2019

Hey, @kares, the breaking change is at bc2ba39ed88096f0d3557231e10bb22a84ee9508.

It seems commit bc2ba39 was done to fix Regression of Ruby 2.4.preview2 string interpolation by @headius. @headius, you know your changes better than I do. Does the removal of the code below affect other tickets? (I'm new to traversing github commits/prs to find issues.)

I found by commenting out the block below (found at Sprintf.java#L1576):

        // round half to even
        if (roundPos + 1 < nDigits && bytes[roundPos + 1] == '5') {
            if ((bytes[roundPos] - '0') % 2 == 0) {
                // round down
                return nDigits;
            }
        }

I was able to run bin/jruby -e 'n = 97.65625; puts format("%f %f %.1f", n, n.round(1), n); puts ("%01.0f" % 1234567892.0)' and return:

97.656250 97.700000 97.7
1234567892

Would this be a viable fix?

@kares

This comment has been minimized.

Copy link
Member Author

kares commented Jan 21, 2019

hey @lassiter, great find - seems like the removal makes sense, at least as a starter.
would propose to do a PR and than we can review the current C-code from there for comparison.

lassiter added a commit to lassiter/jruby that referenced this issue Jan 23, 2019

Added two tests for Sprintf, one based on jruby#5556 and the other ba…
…sed on https://bugs.ruby-lang.org/issues/12889. Corrected a regression issue within Sprintf affecting the JRuby format method for floats. @kares and @aircert collaborated with issue 5556.

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

[fix] only round half-to-even when no tail left
e.g.  0.25 -> 0.2   0.2500 -> 0.2   0.2501 -> 0.3

resolves jrubyGH-5556

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

[test] few round half-to-even asserts for sprintf
since this seems to be a failure point for JRuby (jrubyGH-5556)

@kares kares added this to the JRuby 9.2.6.0 milestone Jan 29, 2019

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

[fix] only round half-to-even when no tail left
e.g.  0.25 -> 0.2   0.2500 -> 0.2   0.2501 -> 0.3

resolves jrubyGH-5556

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

kares added a commit to kares/jruby that referenced this issue Jan 29, 2019

[test] few round half-to-even asserts for sprintf
since this seems to be a failure point for JRuby (jrubyGH-5556)

@enebo enebo closed this in #5580 Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.