Branch prediction in a java for loop

Question

I saw this comment next to a if condition:

// branch prediction favors most often used condition

in the source code of the JavaFX SkinBase class.

protected double computeMinWidth(double height, double topInset, double rightInset, double bottomInset, double leftInset) {

    double minX = 0;
    double maxX = 0;
    boolean firstManagedChild = true;
    for (int i = 0; i < children.size(); i++) {
        Node node = children.get(i);
        if (node.isManaged()) {
            final double x = node.getLayoutBounds().getMinX() + node.getLayoutX();
            if (!firstManagedChild) {  // branch prediction favors most often used condition
                minX = Math.min(minX, x);
                maxX = Math.max(maxX, x + node.minWidth(-1));
            } else {
                minX = x;
                maxX = x + node.minWidth(-1);
                firstManagedChild = false;
            }
        }
    }
    double minWidth = maxX - minX;
    return leftInset + minWidth + rightInset;
}

I believe that the developer want to explain why he wrote a negate if.

is this optimization really useful ?

Answer 1

The guys of the JavaFX team are pretty focused on performance so they surely know that switching the if and the else has no material effect on branch prediction.

I imagine that it is an indication that there is no significant performance improvement to refactor as:

double minX = Double.MAX_VALUE;
double maxX = Double.MIN_VALUE;

for (int i = 0; i < children.size(); i++) {
  Node node = children.get(i);
  if (node.isManaged()) {
    final double x = node.getLayoutBounds().getMinX() + node.getLayoutX();
    minX = Math.min(minX, x);
    maxX = Math.max(maxX, x + node.minWidth(-1));
  }
}

Because branch prediction will essentially turn the if/else into something like that for you (give or take).

---

This commit confirms this explanation, although I'm not sure why they changed it - possibly because it had side effects when the isManaged condition was never true...

Investigating further, the commit refers to a bug "up to 50% performance regression in Controls benchmarks" (you need to login to access it).

It seems that they had a performance issue and while investigating noticed that there was a bug in the code (that was similar to what my example above). They fixed the bug and added a comment to clarify that the fix did not affect performance thanks to branch prediction.

Excerpts:

[..] looking at the code, I noticed an error in the case where none of the skin's children are managed. In such a case, minX/Y and maxX/Y would end up being MAX/MIN_VALUE where we really want them to be zero. So this change set fixes that issue. In testing, I saw a small (~1 fps) improvement in performance, so I don't think this change fixes the performance problem. Regardless, the code must be the way it is.

[...] Note that there was a bug in the use of MIN/MAX - those values are not the largest and smallest values for Float and Double (that would have made sense wrt symmetry with the integral types, but it isn't the way they were specified). For doing min/max operations on floating point values you want to use the NEGATIVE/POSITIVE_INFINITY values instead to achieve the results you are looking for.