bad java compiler optimization?

Question

I have this piece of code:

private void prepareContent() {
    log.info("do something");
    // success?
    boolean suc = false;
    suc = suc || uncompressToContent("file.tar.gz");
    suc = suc || uncompressToContent("file.tgz");
    for (int i = 0; i <= 9; i++) {
        suc = suc || uncompressToContent("dir/" + i + ".tgz");
        suc = suc || uncompressToContent("dir/" + i + ".tar.gz");
    }
    if (!suc) {
        log.error("unable to do something");
    }
}

The function returns false for "file.tar.gz" and file.tgz".

The problem is the the call to uncompressToContent("dir/1.tgz") returns true and the code stops its execution. The remaining code is not executed.

I'm not sure if this is an error in the compiler. What do you think?

Added: I forgot to mention that I need to execute all the calls to uncompressToContent and check if any returns true, using the fewer instructions as possible.

First of all, do not use short-circuiting || but | instead. Secondly, what do you mean it returns false for these two files? Thirdly, have you tried to step through with a debugger? — Hamish Grubijan
– Hamish Grubijan, Commented Jul 15, 2010 at 1:30
Perhaps you should explain (or include comments!) what you think this code is doing (in particular, with the use of the || operator). I suspect you'll find that the meaning of the code does not correspond to what you're intending. — David Gelhar
– David Gelhar, Commented Jul 15, 2010 at 1:35
I have think about this. I think the compiler is doing something like: suc = uncompressToContent("file.tar.gz") || uncompressToContent("file.tgz") || uncompressToContent("...") || ... So, when it finds one true value, the execution is stopped. Is this feature documented? I need to execute the uncompressToContent function 9 times, and I don't want to write several lines of code :( — Javier
– Javier, Commented Jul 15, 2010 at 1:40
When I read the code, I assumed it was working as intended. After the code found the suitable file, it stopped looking. After all, why would you want multiple decompressions of the same files. Just pointless busy work for the computer. If you want it to do the pointless busy work, then you can change the || to |. — emory
– emory, Commented Jul 15, 2010 at 1:43

mikera · Accepted Answer · 2010-07-15 01:32:03Z

10

There is no error in the compiler.

As soon as suc is set to true (i.e. from the first uncompressToContent call) then all of the future expressions will return true without calling uncompressToContent. This is becuase you are using short circuit boolean or ("||") which do not evaluate the second argument if the first argument is true.

If you want all the calls to be made, use the normal or operator ("|") instead.

answered Jul 15, 2010 at 1:32

mikera

107k28 gold badges264 silver badges427 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Uri · Accepted Answer · 2010-07-15 03:38:41Z

If the uncompress method returns true if there was a successful decompression, then suc would become true the first time that this happens. Once suc is true, all the other conditions would be true as soon as suc is evaluated, so the other part of the OR would not be evaluated. Thus, no decompressions will be attempted once at least one is successful.

This is called short circuiting and is the correct behavior and is a very useful property in most languages. And is also not a compiler optimization since it is part of the defined behavior of the language.

Beyond this answer, there are, I think, ways to make this code more readable. First, are you sure that you want to OR rather than AND here? It seems like you want to quit as soon as one file did not compress decorrectly, not stop as one did decompress correctly.

Second, a better design, IMHO, would be to create a list of all the filenames you want to decompress, and then do a for-each over that list and do all the decompressions, it would make things more readable.

Third, if in most cases decompression would be successful, I think that exception handling is much better than boolean return values.

Here is how I would write something like this (and I would break it into functions)

List<String> filenames = new ArrayList<String>();

this.collectFilenamesToDecompress(filenames) // Write one or more than one functions of this sort based on the semantics of your problem

try
{
   for(String filename: filenames)
   {
      uncompressFile(filename); // This will throw an exception if there is a failure
   } 
} catch(Exception e)
{
    // Announce that there was an error and you stopped decompressing because there was an error.
    // Return or quit
}
// If you got here, everything is great!

Uri, I just filed a bug at work about a command-line tool that prints nothing upon success and nothing upon a failure. It does, however, return a -1 on exit :). If I tell you where the author lives, will you bring a baseball bat?
@Hamish: I'm a big believer in the three stooges approach to teamwork :) And I totally expect to have a pie thrown in my face for some of my code.
this was real code. My code :) I knew about s = a | b | c | d But never seen this in an unrolled loop. How would you solve this problem with few lines of code?
@Javier: Ah. I hope I didn't offend. Folks sometimes use this feature to show off or to write convulted interview questions. More to the point, I think that what you're trying to do is to make sure that all of them succeeded, not just one, and stop if there is at least one failure. Using and might be better for you here. I edited my question with some ways to solve this.
@Javier I'm confused. Do you want the short-circuited behavior or not?

SLaks · Accepted Answer · 2010-07-15 01:29:16Z

4

This behavior is by design.

Logical operators in most languages are short-circuiting.

In the expression a || b, b will only be evaluated if a is false.

Therefore, once suc becomes true, none of the other calls to uncompressToContent will be evaluated.

answered Jul 15, 2010 at 1:29

SLaks

891k182 gold badges1.9k silver badges2k bronze badges

Comments

Stephen C · Accepted Answer · 2010-07-15 02:14:13Z

I think the compiler is doing something like: suc = uncompressToContent("file.tar.gz") || uncompressToContent("file.tgz") || uncompressToContent("...") || ... So, when it finds one true value, the execution is stopped. Is this feature documented?

Yes. It is clearly documented in the Java Language Specification section 15.24, where it says this:

"The || operator is like | (§15.22.2), but evaluates its right-hand operand only if the value of its left-hand operand is false."

The JLS then goes on to explain exactly what happens in excruciating detail. Follow the link above if you are interested.

Oh yea, and in this respect the Java || operator behaves the same as in C, C++, C#, Perl and many other programming languages.

OMG.. you actually provided me a good reference :D did you read the java lang. spec.? congratulations. Thanks for taking the time to answer my question.
"Did you read the JLS" - Not cover to cover, but I do consult it occasionally when I want a definitive answer.

Collectives™ on Stack Overflow

bad java compiler optimization?

4 Answers 4

Comments

8 Comments

Comments

2 Comments

Hot Network Questions

Collectives™ on Stack Overflow

4 Answers 4

Comments

8 Comments

Comments

2 Comments

Related