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

Convert to BigDecimals #72

Closed
lokka30 opened this issue Dec 15, 2021 · 17 comments
Closed

Convert to BigDecimals #72

lokka30 opened this issue Dec 15, 2021 · 17 comments
Assignees
Labels
priority: high High priority status: confirmed Approved / validated type: improvement A feature is added or adjusted

Comments

@lokka30
Copy link
Member

lokka30 commented Dec 15, 2021

I am unsure whether we should use double or BigDecimal in our Economy API. Does anyone have arguments for/against BigDecimal?

Our current use of double was questioned here.

Double

  • Sufficient precision (approx. 16 decimal digits).
  • Higher performance.

BigDecimal

  • Doesn't suffer from this interesting problem.
@lokka30 lokka30 added the thoughts wanted Thoughts from other developers would be appreciated on this issue. label Dec 15, 2021
@lokka30 lokka30 self-assigned this Dec 15, 2021
@lokka30 lokka30 added the type: improvement A feature is added or adjusted label Dec 16, 2021
@MrIvanPlays
Copy link
Member

We don't need that much precision so that's a big no from me.

@MrNemo64
Copy link
Contributor

I suppose that we could change double parameter types for Number so BigDecimal, BigInteger, Double, Integer, Long, etc work

@MrIvanPlays
Copy link
Member

I suppose that we could change double parameter types for Number so BigDecimal, BigInteger, Double, Integer, Long, etc work

that escalated

i dont know

@lokka30
Copy link
Member Author

lokka30 commented Dec 18, 2021

Although doubles have greater performance, it is important to consider that this is handling money, so precision is important. Doubles can store sufficient amounts of decimal places, but when it comes to calculations, doubles are imprecise.

This article is more than a decade old, however, it does have an interesting performance comparison in it (in case you are interested in the numbers).

This leaves the final competition to: BigDecimals have greater precision, but are much (relatively) slower to use. Vice-versa for doubles.

My current standing has changed from using doubles to BigDecimals, but I am still very interested in what you all think, before I make the changes.

@tatteaid
Copy link

I would recommend you all take a read of this.

Although doubles can store more than enough decimal digits, it is imprecise in calculations. This really leaves it to 'doubles have greater performance', and 'bigdecimals have greater precision', to compare the two. Which should we choose?

I am yet to find a decent article comparing the performance of BigDecimals and doubles, perhaps this is just a nitpick and BigDecimals are barely slower than doubles. I'll try find some information on this.

I mean, it really comes down to a few things.

Do you really want/need this level of precision? If so, you're going to either have to find a work-around solution when using a double or float (which still probably will not be full-proof), create your own way of storing precise amounts of money which may or may not be faster then a BigDecimal, or use a BigDecimal. Although BigDecimal's are significantly slower than doubles/floats, I don't think it's absolutely critical if implemented correctly. If you're concerned, you can run your own benchmarks on it.

You're not going to be running BigDecimal operations 250,000 times, and even if you were those times (if accurate) are still very fast (and you could always just have a callback or use some sort of CompleteableFuture). A list of balances would be paged, so you still wouldn't get close to that many entries. And if you were to somehow need to do something like that onEnable or onDisable, it doesn't really matter (unless of course, it takes an exceptionally long time).

I think it really just comes down to if you want to or not. It's going to be more precise than a double/float, which is the usual for handling currencies.

@lokka30
Copy link
Member Author

lokka30 commented Dec 18, 2021

I would recommend you all take a read of this.
Although doubles can store more than enough decimal digits, it is imprecise in calculations. This really leaves it to 'doubles have greater performance', and 'bigdecimals have greater precision', to compare the two. Which should we choose?
I am yet to find a decent article comparing the performance of BigDecimals and doubles, perhaps this is just a nitpick and BigDecimals are barely slower than doubles. I'll try find some information on this.

I mean, it really comes down to a few things.

Do you really want/need this level of precision? If so, you're going to either have to find a work-around solution when using a double or float (which still probably will not be full-proof), create your own way of storing precise amounts of money which may or may not be faster then a BigDecimal, or use a BigDecimal. Although BigDecimal's are significantly slower than doubles/floats, I don't think it's absolutely critical if implemented correctly. If you're concerned, you can run your own benchmarks on it.

You're not going to be running BigDecimal operations 250,000 times, and even if you were those times (if accurate) are still very fast (and you could always just have a callback or use some sort of CompleteableFuture). A list of balances would be paged, so you still wouldn't get close to that many entries. And if you were to somehow need to do something like that onEnable or onDisable, it doesn't really matter (unless of course, it takes an exceptionally long time).

I think it really just comes down to if you want to or not. It's going to be more precise than a double/float, which is the usual for handling currencies.

I like your thoughts here. The lower performance of BigDecimal ought to be highly insignificant for all if not most applications implementing the Economy API.
There are ways to optimize the speed of BigDecimals as well. Caching them if they're being used later is one thing. Again, this only concerns the 250,000 operation scenarios.

@lokka30
Copy link
Member Author

lokka30 commented Dec 18, 2021

And, if an economy system desires extreme precision, the use of BigDecimal will allow that.

@tatteaid
Copy link

tatteaid commented Dec 18, 2021

I would recommend you all take a read of this.
Although doubles can store more than enough decimal digits, it is imprecise in calculations. This really leaves it to 'doubles have greater performance', and 'bigdecimals have greater precision', to compare the two. Which should we choose?
I am yet to find a decent article comparing the performance of BigDecimals and doubles, perhaps this is just a nitpick and BigDecimals are barely slower than doubles. I'll try find some information on this.

I mean, it really comes down to a few things.
Do you really want/need this level of precision? If so, you're going to either have to find a work-around solution when using a double or float (which still probably will not be full-proof), create your own way of storing precise amounts of money which may or may not be faster then a BigDecimal, or use a BigDecimal. Although BigDecimal's are significantly slower than doubles/floats, I don't think it's absolutely critical if implemented correctly. If you're concerned, you can run your own benchmarks on it.
You're not going to be running BigDecimal operations 250,000 times, and even if you were those times (if accurate) are still very fast (and you could always just have a callback or use some sort of CompleteableFuture). A list of balances would be paged, so you still wouldn't get close to that many entries. And if you were to somehow need to do something like that onEnable or onDisable, it doesn't really matter (unless of course, it takes an exceptionally long time).
I think it really just comes down to if you want to or not. It's going to be more precise than a double/float, which is the usual for handling currencies.

I like your thoughts here. The lower performance of BigDecimal ought to be highly insignificant for all if not most applications implementing the Economy API. There are ways to optimize the speed of BigDecimals as well. Caching them if they're being used later is one thing. Again, this only concerns the 250,000 operation scenarios.

Yeah, although I still can't think of a way where you'd have that many BigDecimal computations or stored, let alone a few thousand.

I think you'll be fine with a BigDecimal implementation, and it'll make your life a lot easy when it comes to precision (I've dealt with float/double precision issues before and it's a waste of time to deal with).

@MrIvanPlays
Copy link
Member

Wtf is wrong with you. It's just an economy plugin for a game we don't need that much precision. Fairly sure that no one will use more than 4 digits after the dot, and if anyone uses more then it's their problem (some crazy psycho)

@MrNemo64
Copy link
Contributor

Wtf is wrong with you. It's just an economy plugin for a game we don't need that much precision. Fairly sure that no one will use more than 4 digits after the dot, and if anyone uses more then it's their problem (some crazy psycho)

I'd say the real question is what is your problem? Why must you have such unnecessary and rude responses? You have 3 comments on this issue, one of them is you laughing at my idea and the other one is you straight up asking what is wrong with someone. Only one of your comments bring anything to the issue and it's something that @lokka30 was able to, not only say, but also bring comparisons and showed some kind of research to make the best decision. You just assume that no-one will ever need so much precision but who knows? There are servers that at the end of their season have real word money prices for some things, for example the richest faction, the player that plays the most, etc... For same cases the best precision may be wanted.
What is the difference between doing

double amount = ...
EconomyTransactionInitiator<?> initiator = ...
Currency currency = ...
EconomySubscriber<Double> subscription = ...
Account account = ...
account.setBalance(amount, initiator, currency, subscription);

and doing

double amount = ...
EconomyTransactionInitiator<?> initiator = ...
Currency currency = ...
EconomySubscriber<Double> subscription = ...
Account account = ...
account.setBalance(new BigDecimal(amount), initiator, currency, subscription);

So I just want to know, why do you just act so bad to any kind of idea that you don't like? @tatteaid just exposed an issue that they thought that should be at least discussed, @lokka30 did their research and thought that maybe @tatteaid has a point.

Now, going back on the topic of this issue, how many operations are needed anyways? A substraction for withdraw, addition for deposit, a multiplication and a division for converting between currencies, maybe some more. How much time does it take for an operation to happend?

import java.math.BigDecimal;
import java.math.RoundingMode;
import java.util.OptionalDouble;
import java.util.Random;
import java.util.stream.LongStream;

public class SpeedTest {

    public long testDouble(double valA, double valB, double valC) {
        double a = valA;
        double b = valB;
        double c = valC;
        long start = System.nanoTime();
        double x1 = a + b;
        double x2 = x1 + c;
        double x3 = a - b;
        double x4 = x3 - c;
        double x5 = a * b;
        double x6 = x5 / c;
        long end = System.nanoTime();
        return (end - start);
    }

    public long testBigDecimal(double valA, double valB, double valC) {
        BigDecimal a = new BigDecimal(valA);
        BigDecimal b = new BigDecimal(valB);
        BigDecimal c = new BigDecimal(valC);
        long start = System.nanoTime();
        BigDecimal x1 = a.add(b);
        BigDecimal x2 = x1.add(c);
        BigDecimal x3 = a.subtract(b);
        BigDecimal x4 = x3.subtract(c);
        BigDecimal x5 = a.multiply(b);
        BigDecimal x6 = x5.divide(c, RoundingMode.HALF_UP);
        long end = System.nanoTime();
        return (end - start);
    }

    public void doTest() {
        final Random r = new Random();
        final int maxVal = 1_000_000;
        final int iterations = 10;
        final long[] bigDecimalTimes = new long[iterations];
        final long[] doubleTimes = new long[iterations];
        for (int i = 0; i < iterations; i++) {
            double valA = r.nextDouble() * r.nextInt(maxVal);
            double valB = r.nextDouble() * r.nextInt(maxVal);
            double valC = r.nextDouble() * r.nextInt(maxVal);
            if(valC == 0d) {
                valC = 1d;
            }

            long bigDecimalTime = testBigDecimal(valA, valB, valC);
            long doubleTime = testDouble(valA, valB, valC);

            bigDecimalTimes[i] = bigDecimalTime;
            doubleTimes[i] = doubleTime;

            System.out.println("BigDecimal test:\t" + bigDecimalTime + "ns\t(" + (bigDecimalTime / 1000) + "ms)");
            System.out.println("Double test:\t\t" + doubleTime + "ns\t(" + (doubleTime / 1000) + "ms)");
        }
        OptionalDouble oAverageBigDecimalTime = LongStream.of(bigDecimalTimes).average();
        OptionalDouble oAverageDoubleTime = LongStream.of(doubleTimes).average();
        if (oAverageDoubleTime.isPresent() && oAverageBigDecimalTime.isPresent()) {
            double averageBigDecimalTime = oAverageBigDecimalTime.getAsDouble();
            double averageDoubleTime = oAverageDoubleTime.getAsDouble();
            System.out.println("Average BigDecimal time:\t" + averageBigDecimalTime + "ns\t(" + (averageBigDecimalTime / 1000) + "ms)");
            System.out.println("Average double time:\t\t" + averageDoubleTime + "ns\t(" + (averageDoubleTime / 1000) + "ms)");
            System.out.println("double is " + (averageBigDecimalTime / averageDoubleTime) + " times faster than BigDecimal");
        }
    }

}

This class gave me this results for 10 iterations:

BigDecimal test:	453100ns	(453ms)
Double test:		200ns	(0ms)
BigDecimal test:	50800ns	(50ms)
Double test:		200ns	(0ms)
BigDecimal test:	47600ns	(47ms)
Double test:		200ns	(0ms)
BigDecimal test:	32100ns	(32ms)
Double test:		200ns	(0ms)
BigDecimal test:	34100ns	(34ms)
Double test:		200ns	(0ms)
BigDecimal test:	68600ns	(68ms)
Double test:		100ns	(0ms)
BigDecimal test:	30200ns	(30ms)
Double test:		100ns	(0ms)
BigDecimal test:	33300ns	(33ms)
Double test:		200ns	(0ms)
BigDecimal test:	34200ns	(34ms)
Double test:		100ns	(0ms)
BigDecimal test:	34500ns	(34ms)
Double test:		100ns	(0ms)
Average BigDecimal time:	81850.0ns	(81.85ms)
Average double time:		160.0ns	(0.16ms)
double is 511.5625 times faster than double

Doing a 1.000.000 iterations test gave me this

Average BigDecimal time:	876.3121ns	(0.8763121ms)
Average double time:		29.6803ns	(0.0296803ms)
double is 29.525041862784406 times faster than BigDecimal

So with these numbers in mind, do we change from double to BigDecimal?

As I said above, if instead of double we use Number, any number can be given as an argument. Then the provider could do

void setBalance(Number n, EconomyTransactionInitiator<?> initiator, Currency currency, EconomySubscriber<Double> subscription) {
  BigDecimal amount;
  if(n instanceof BigDecimal) {
    amount = (BigDecimal) n;
  } else {
    amount = new BigDecimal(n.doubleValue());
  }
  ...
}

if he wants to work with BigDecimal. If the provider doesn't care about pecision then they can just do

void setBalance(Number n, EconomyTransactionInitiator<?> initiator, Currency currency, EconomySubscriber<Double> subscription) {
  double amount = n.doubleValue();
  ...
}

For a method that returns an amount of money returning Number doesn't seem that good of an idea but a solution could be thought.
The other option is just to switch to BigDecimal.

@MrIvanPlays
Copy link
Member

@MrNemo64 im sorry, was drunk.

Anyways, initiating a new object for specifying a number is not really for me. Perhaps if you really think bigdecimal is needed i'd like helper methods which accept double.

@lokka30
Copy link
Member Author

lokka30 commented Dec 18, 2021

I think simply switching to BigDecimal altogether is fine - you can use BigDecimal#valueOf(double) and BigDecimal#doubleValue, if I remember correctly @MrIvanPlays :) I doubt any helper methods are necessary due to that.

@Jikoo
Copy link
Collaborator

Jikoo commented Dec 18, 2021

IMO all the arguments boil down to this:
Pro - Precision:

  • Point: Encourages implementation and usage to be precise
  • Counterpoint: Implementation can already be as precise as it would like internally
    • Does not address bad usages performing imprecise math and setting balance

Con - Speed:

  • Point: BD is slower.
  • Counterpoint: Treasury makes it very clear that transactions will take time. Who cares if some of that time is due to object creation and math instead of database latency?

Treasury is seeking to be the best API. It's not seeking to be the most blazingly fast economy implementation. Yes, BigDecimal may be overkill, but then again, so is microoptimizing the speed of math involved. Treasury makes it clear transactions may take time, and if some of that time is object creation and processing math instead of database latency, why does that matter? Which does Treasury want to support better, a plugin that charges you for rotating your head listening to the player move event or a stock exchange that allows trading fractional shares of valuable stock? End users already don't understand floating point precision and pretty often wonder why simple math ends them with 1.0000005 instead of 1.

The counterpoint is that the economy implementation is free to use BD for its backend - numbers exposed to users will probably be pre-rounded to a certain number of places anyway, but internal tracking does not have to suffer for it. That said, since Treasury does not force users of the API to do precise operations, if one were to do something like get balance, do math, then set balance rather than attempt to add/subtract using the API methods, the precision loss would still be there.

I initially was against using BD, but having now spent time typing up a big thing about the pros and cons I support it.

@lokka30
Copy link
Member Author

lokka30 commented Dec 18, 2021

Great points there Jikoo. I will give it a day or so and if there is still general support for the change then I will go forward with it.

@creatorfromhell
Copy link
Contributor

I'd also like to bring up the really relevant tid bit that the majority of economy plugins utilize BigDecimal. This includes everything from Essentials' built-in economy system to the Sponge economy API itself.

@lokka30
Copy link
Member Author

lokka30 commented Dec 19, 2021

Once there are no PRs open I will work on solving this issue.

@MrNemo64
Copy link
Contributor

@MrNemo64 im sorry, was drunk.

It's alright, let's try to keep these comments out.

I'm ok with converting to BigDecimal

@lokka30 lokka30 added status: confirmed Approved / validated priority: high High priority and removed thoughts wanted Thoughts from other developers would be appreciated on this issue. labels Dec 20, 2021
creatorfromhell added a commit to creatorfromhell/Treasury that referenced this issue Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high High priority status: confirmed Approved / validated type: improvement A feature is added or adjusted
Projects
None yet
Development

No branches or pull requests

6 participants