current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite

I'm kind of new to Java but I am writing an Android app. Right now I'm working on an async task to calculate Pi but when I run it the memory usage increases alarmingly (+5MB per second). This one piece of code may need to run thousands of times so it is important that it be very optimized.

protected BigDecimal doInBackground(Object... params) {
        digits = (int)params[0];
        piView = (TextView)params[1];
        boolean add = true;
        BigDecimal pi = new BigDecimal(0.0);
        for(int i=1; pi.setScale(digits, BigDecimal.ROUND_DOWN).equals(realPi) == false; i++){
            if(add){
                pi = pi.add(new BigDecimal(4.0 / (-1 + (i*2))));
                add = false;
            }
            else {
                pi = pi.subtract(new BigDecimal(4.0 / (-1 + (i*2))));
                add = true;
            }
        }
        return pi;
    }

This one piece of code may need to run thousands of times so it is important that it be very optimized.

share|improve this question
    
Also, does your code actually computes pi, or is it stuck eating up memory? – Mathias Ettinger Nov 9 at 0:01
    
It will compute pi given enough time (also thanks I haven't used code review before, i'll edit the title) – TimTheEnchanter Nov 9 at 0:02
    
I don't know what is causing the memory leak, but the if loop would probably be clearer as a while, since the test condition doesn't use "i". – Phil Freihofner Nov 9 at 0:13
    
I think for works fine because I'd have to make a count variable anyway – TimTheEnchanter Nov 9 at 0:28
    
Why not use double primitive type all the way? – bowmore Nov 9 at 10:13
up vote 3 down vote 
    boolean add = true;
    for(int i=1; pi.setScale(digits, BigDecimal.ROUND_DOWN).equals(realPi) == false; i++){
        if(add){
            pi = pi.add(new BigDecimal(4.0 / (-1 + (i*2))));
            add = false;
        }
        else {
            pi = pi.subtract(new BigDecimal(4.0 / (-1 + (i*2))));
            add = true;
        }
    }

You don't need an add variable. Consider 

    int i = 1;
    while (!pi.setScale(scale, BigDecimal.ROUND_DOWN).equals(realPi)) {
        pi = pi.add(new BigDecimal(4.0 / i));
        i += 2;
        pi = pi.subtract(new BigDecimal(4.0 / i));
        i += 2;
    }

This may do one extra subtract operation, but it does half as many equals operations. And it doesn't waste time on setting and checking a variable just to alternate between two operations.

We don't need the (-1 + (i*2)) part. If we increment by 2 instead of 1, we get the same stream of values with strictly less math. 1, 3, 5, 7, 9, 11, ...

I changed the == false to the more idiomatic !.

I also changed digits to scale as I had trouble reading the original.

BigDecimal gives precise results, not quick results. It is quite possible that you are hitting real limits of the format. It's unclear at the moment why you are calculating \$\pi\$. With more context, we might be able to give more help.

share|improve this answer
    
While this is a much neater and concise way of writing the code it does not in fact slow the memory consumption at all – TimTheEnchanter Nov 9 at 21:29
up vote 1 down vote

In addition to @mdfst13's excellent answer.

I see nothing that could leak memory. However you are allocating a lot of new BigDecimal objects in a tight loop and then throwing them away. These must sooner or later be garbage collected by the VM. The GC sees that it has plenty of memory available in the system and is thus postponing its work of cleaning up so that your program can run quicker, it will clean up the memory when it feels that there is a need to do so.

In other words, things are working as normal. Do not be alarmed by the memory usage.

share|improve this answer
    
While I do think it might be creating the new BigDecimals each time that is creating the problem the Garbage collector is not doing its job because the allocated memory increases until the app crashes. – TimTheEnchanter Nov 9 at 21:28

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.