Fix Time subsecond precision loss from floating point arithmetic#9261
Merged
headius merged 2 commits intojruby:jruby-10.0from Mar 4, 2026
Merged
Fix Time subsecond precision loss from floating point arithmetic#9261headius merged 2 commits intojruby:jruby-10.0from
headius merged 2 commits intojruby:jruby-10.0from
Conversation
Use integer arithmetic when computing millis/nanos from Rational Fixes Psych::Visitors::TestToRuby#test_time https://github.com/jruby/jruby/actions/runs/22045652593/job/63693879405
30ca982 to
539085c
Compare
Member
|
Those failures are not your fault. Master merged 10.1 and something needs to be tweaked. Thank you! I have been running into this occasionally on our CI but never pinned it down. |
Member
|
Suggested change to eliminate one diff --git a/core/src/main/java/org/jruby/RubyTime.java b/core/src/main/java/org/jruby/RubyTime.java
index d6a99b6450..5add2109db 100644
--- a/core/src/main/java/org/jruby/RubyTime.java
+++ b/core/src/main/java/org/jruby/RubyTime.java
@@ -113,6 +113,7 @@ public class RubyTime extends RubyObject {
private static final BigDecimal ONE_MILLION_BD = BigDecimal.valueOf(1000000);
private static final BigDecimal ONE_BILLION_BD = BigDecimal.valueOf(1000000000);
public static final int TIME_SCALE = 1_000_000_000;
+ public static final BigInteger TIME_SCALE_BI = BigInteger.valueOf(RubyTime.TIME_SCALE);
public static final int TIME_SCALE_DIGITS = 9;
private DateTime dt;
diff --git a/core/src/main/java/org/jruby/util/time/TimeArgs.java b/core/src/main/java/org/jruby/util/time/TimeArgs.java
index eaed31c7e5..fb445f231f 100644
--- a/core/src/main/java/org/jruby/util/time/TimeArgs.java
+++ b/core/src/main/java/org/jruby/util/time/TimeArgs.java
@@ -136,7 +136,7 @@ public class TimeArgs {
}
long subSeconds = BigInteger.valueOf(numerator)
- .multiply(BigInteger.valueOf(TIME_SCALE))
+ .multiply(RubyTime.TIME_SCALE_BI)
.divide(BigInteger.valueOf(denominator))
.longValue();
There's still a lot of narrowing conversions in this code... is there a reason why we shouldn't just use BigInteger throughout? Maybe there's a precision threshold we've already met with your logic, but going back and forth between longs and BigInteger makes me wonder. 🤔 |
96d3c41 to
0ff29d1
Compare
Member
|
@evaniainbrooks You still have this marked as draft. Is there any more? |
Contributor
Author
|
I had another commit prepared to use BigInteger throughout, but, after reasoning through it, it does seem that this is enough. |
Contributor
Author
|
@headius marked as ready |
Member
|
Thank you! I have some refactoring ideas that I'll push as a separate PR. |
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use integer arithmetic when computing millis/nanos from Rational
Fixes Psych::Visitors::TestToRuby#test_time
https://github.com/jruby/jruby/actions/runs/22045652593/job/63693879405