Как-то при небольшой загрузке задачами тимлид дал нам интересную задачу на развитие: прочитать основные классы 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);
}
}
А за подробностями можете заглянуть в багтрекер. Но там, кажется, просто отмахнулись по принципу «работает — не трогай» (читай последний коммент с умным словом «идиосинкразия») 🙁
Заключение
Большинство этих недочётов устранено в новых версиях (хотя судя по всему эти методы были в принципе переделаны, и этот код переписан 😉 ) Но некоторые, в частности лишняя проверка в хэш мап, остаются до сих пор.
Зарепорть в баг-трекер и будет тебе вечная слава!