Баги в OpenJDK 1.8 |A| alse-code

Как-то при небольшой загрузке задачами тимлид дал нам интересную задачу на развитие: прочитать основные классы java core + collections. А тому, кто найдёт ошибку в HashMap, обещал штуку рублей, что придало хороший азартный блеск нашим глазам 🙂

Что? Ошибки в JDK? Такое бывает? Да (посмотрите хотя бы их багтрекер). А найти парочку самому неплохо повышает самоуверенность, так что всем советую;)

Вызов принят, и кроме искомой ошибки в HashMap, я нашел ещё несколько, о чём и хочу рассказать.

java.lang.AbstractStringBuilder#appendNull

private AbstractStringBuilder appendNull() {
    int c = count;
    ensureCapacityInternal(c + 4);
    final char[] value = this.value;
    value[c++] = 'n';
    value[c++] = 'u';
    value[c++] = 'l';
    value[c++] = 'l';
    count = c;
    return this;
}

Зачем здесь переменная c? Ведь можно было бы сразу использовать count. Лишняя переменная, хм, может она для многопоточности? Незачем — в наследниках этого класса, StringBuffer, сделано всё необходимое для многопоточности — вызов методов предков просто обёрнут с synchronized модификатором (в отличии от StringBuilder)

Плохо ли это, можно ли вообще назвать багом? Пришлось как следует поискать и почитать, чтобы понять, зачем это. Оказывается, это сделано специально для оптимизации — при копировании в локальные переменные метода байткод становится чуточку меньше. А когда меньше байткод — это влияет, например, и на решение JVM о встраивании метода в место вызова (inline). Почти везде в Core классах происходит такая микро-оптимизация. Подробнее об этом можно прочитать 1, 2, 3. Однако не рекомендуют использовать подобные оптимизации в своём коде — это затрудняет чтение, а оптимизацию даёт минимальную, которую, к тому же, может выполнить JVM после нескольких проходов.

Хорошо, значит так даже лучше. Ок, но тогда если посмотрим метод java.lang.AbstractStringBuilder#append(boolean) — то он полностью аналогичен, хоть и там сделано без этой микро-оптимизации:

public AbstractStringBuilder append(boolean b) {
        if (b) {
            ensureCapacityInternal(count + 4);
            value[count++] = 't';
            value[count++] = 'r';
            value[count++] = 'u';
            value[count++] = 'e';
        } else {
            ensureCapacityInternal(count + 5);
            value[count++] = 'f';
            value[count++] = 'a';
            value[count++] = 'l';
            value[count++] = 's';
            value[count++] = 'e';
        }
        return this;
    }

Тогда, получается, настоящий «баг» (а точнее, просто несоответствие стиля и отсутствие оптимизаций) — здесь.

Отдавая должное — в новых версиях джавы это поправили 😊

java.util.LinkedList.LLSpliterator#getEst

final int getEst() {
    int s; // force initialization
    final LinkedList<E> lst;
    if ((s = est) < 0) {
        if ((lst = list) == null)
            s = est = 0;
        else {
            expectedModCount = lst.modCount;
            current = lst.first;
            s = est = lst.size;
        }
    }
    return s;
}

Тут сразу два момента. Если мы присмотримся, переменная s здесь тоже кажется лишней. Правда там оставили коммент — возможно, это для каких-то оптимизаций на уровне байткода..

Ок, второй момент. list здесь не может быть null — он проставляется только в конструкторе LLSpliterator, и только один раз, и точно не null:

public Spliterator<E> spliterator() {
    return new LLSpliterator<E>(this, -1, 0);
}

ссылку на метод, приводимую в начале раздела, вы можете вставить в InelliJ IDEA по shift-shift (нужно только отметить в открывшемся окне include non-project items)

То есть первая ветка if-а никогда не выполняется. Вполне возможно, что это не оптимизируется ни компилятором, ни во время выполнения.

Конструктор обявлен как package-private — кто знает, может это сделано на случай, если кто-нибудь сделает себе пэкэдж java.util(или отнаследуется) и вызовет от туда конструктор, передав null — но, согласитесь, «такое себе». Думаю, лучше убрать лишнюю ветку и объявить конструктор как private.

java.util.ArrayList#removeIf

public boolean removeIf(Predicate<? super E> filter) {
    int removeCount = 0;
    final BitSet removeSet = new BitSet(size);
    final int expectedModCount = modCount;
    final int size = this.size;
    for (int i=0; modCount == expectedModCount && i < size; i++) {
        final E element = (E) elementData[i];
        if (filter.test(element)) {
            removeSet.set(i);
            removeCount++;
        }
    }
    if (modCount != expectedModCount) {
        throw new ConcurrentModificationException();
    }

    // shift surviving elements left over the spaces left by removed elements
    final boolean anyToRemove = removeCount > 0;
    if (anyToRemove) {
        final int newSize = size - removeCount;
        for (int i=0, j=0; (i < size) && (j < newSize); i++, j++) {
            i = removeSet.nextClearBit(i);
            elementData[j] = elementData[i];
        }
        for (int k=newSize; k < size; k++) {
            elementData[k] = null;  // Let gc do its work
        }
        this.size = newSize;
        if (modCount != expectedModCount) {
            throw new ConcurrentModificationException();
        }
        modCount++;
    }

    return anyToRemove;
}

Метод служит для удаления элементов ArrayList, удовлетворяющих условию.

Тут посложнее. В первом цикле все элементы прогоняются по предикату и индексы удаляемых проставляются в BitSet. Во втором цикле берётся nextClearBit, который возвращает следующий true бит — то есть, удалённые элементы перезатираются новыми. Но i при этом смещается, пропуская удалённые элементы — таким образом, к концу цикла i станет равным size, при этом j станет равным newSize, так как идёт по количеству элементов, минус количество удаляемых, а i пропускает эти удаляемые. Таким образом, сравнение обоих здесь не нужно. Можно оставить (i < size) либо (j < newSize)

java.util.HashMap#treeifyBin

Это и есть искомый недочёт, который нашел наш тимлид (+1000rub achieved). Здесь я предлагаю вам самостоятельно найти лишнюю проверку.

final void treeifyBin(Node<K,V>[] tab, int hash) {
    int n, index; Node<K,V> e;
    if (tab == null || (n = tab.length) < MIN_TREEIFY_CAPACITY)
        resize();
    else if ((e = tab[index = (n - 1) & hash]) != null) {
        TreeNode<K,V> hd = null, tl = null;
        do {
            TreeNode<K,V> p = replacementTreeNode(e, null);
            if (tl == null)
                hd = p;
            else {
                p.prev = tl;
                tl.next = p;
            }
            tl = p;
        } while ((e = e.next) != null);
        if ((tab[index] = hd) != null)
            hd.treeify(tab);
    }
}

А за подробностями можете заглянуть в багтрекер. Но там, кажется, просто отмахнулись по принципу «работает — не трогай» (читай последний коммент с умным словом «идиосинкразия») 🙁

Заключение

Большинство этих недочётов устранено в новых версиях (хотя судя по всему эти методы были в принципе переделаны, и этот код переписан 😉 ) Но некоторые, в частности лишняя проверка в хэш мап, остаются до сих пор.