Шутки шутками и срачи срачами а у меня есть пара логичных придирок.
1) Почему вместо использования Caffeine cache, как альтернативы старой гуавы (
Авторизуйтесь для просмотра ссылок.
), что было бы логично, в форке написана своя собственная реализация на конкурентных мапах. Работает я уверен это хуже, чем даже оригинальная гуава.
2) Если у нас форк нацелен на оптимизацию - почему до сих пор продолжается использование stream api даже в тех моментах, которые были изменены?
Банальный пример (первый попавшийся при сравнении):
List<LocalPlayer> wgPlayers = Bukkit.getServer().getOnlinePlayers().stream().map(player -> WorldGuardPlugin.inst().wrapPlayer((Player)player)).toList();
В оригинале там легаси collect(Collectors.toList());, так что замена очевидно была (при помощи инспеций Idea, пойдёт), но можно было бы пойти дальше и заменить на обычные циклы, что к слову было реализовано например в методе getBlocks, получается оптимизация какая-то выборочная
3) Иногда применение инспеций Idea не всегда хорошо
Очевидно - не стоило добавлять эти Objects.requireNotNull итп
Но другой момент - замена get(0) на getFisrt не имела смысла, поскольку под капотом он выполняет всю ту же работу, только добавляет околобессмысленную в нашей ситуации проверку на пустоту листа
Java:
default E getFirst() {
if (this.isEmpty()) {
throw new NoSuchElementException();
} else {
return this.get(0);
}
}
В целом get(0) при пустом листе и так и так бы высрал нам эксепшн, так что шило на мыло меняем (я знаю, что НУ ОЧЕВИДНО это влияет на производительность ПОЧТИ никак, но если заниматься байтоблядсвом то заниматься на полную)
4) Не ясно зачем было вырезать блеклист из WG целиком
Да, фича не самая полезная, но всё же может кто-то юзал! (как я)
5) Некоторые оптимизации не имеют смысла как таковые
Пример:
Java:
public RegionResultSet(Set<ProtectedRegion> applicable, @Nullable ProtectedRegion globalRegion) {
this(NormativeOrders.fromSet(applicable), globalRegion, true);
this.regionSet = switch (applicable.size()) {
case 0 -> Collections.emptySet();
case 1 -> Collections.singleton(applicable.iterator().next());
default -> ImmutableSet.copyOf(applicable);
};
}
Что тут не так?
Реализация copy у ImmutableSet
Java:
public static <E> ImmutableSet<E> copyOf(Collection<? extends E> elements) {
/*
* TODO(lowasser): consider checking for ImmutableAsList here
* TODO(lowasser): consider checking for Multiset here
*/
// Don't refer to ImmutableSortedSet by name so it won't pull in all that code
if (elements instanceof ImmutableSet && !(elements instanceof SortedSet)) {
@SuppressWarnings("unchecked") // all supported methods are covariant
ImmutableSet<E> set = (ImmutableSet<E>) elements;
if (!set.isPartialView()) {
return set;
}
} else if (elements instanceof EnumSet) {
EnumSet<?> clone = ((EnumSet<?>) elements).clone();
ImmutableSet<?> untypedResult = ImmutableEnumSet.asImmutable(clone);
/*
* The result has the same type argument we started with. We just couldn't express EnumSet<E>
* or ImmutableEnumSet<E> along the way because our own <E> isn't <E extends Enum<E>>.
*
* We are also performing a safe covariant cast to change <? extends E> to <E>.
*/
@SuppressWarnings("unchecked")
ImmutableSet<E> result = (ImmutableSet<E>) untypedResult;
return result;
}
if (elements.isEmpty()) {
// We avoid allocating anything.
return of();
}
// Collection<E>.toArray() is required to contain only E instances, and all we do is read them.
// TODO(cpovirk): Consider using Object[] anyway.
@SuppressWarnings("unchecked")
E[] array = (E[]) elements.toArray();
/*
* For a Set, we guess that it contains no duplicates. That's just a guess for purpose of
* sizing; if the Set uses different equality semantics, it might contain duplicates according
* to equals(), and we will deduplicate those properly, albeit at some cost in allocations.
*/
int expectedSize =
elements instanceof Set ? array.length : estimatedSizeForUnknownDuplication(array.length);
return fromArrayWithExpectedSize(array, expectedSize);
}
Тобиш под капотом он точно так же проверяет на то не является ли коллекция, которую мы копируем пустой, а если является - возвращаем ImmutalbeList.of, который возвращает уже единый экземпляр RegularImmutableSet.EMPTY
Тобиш в итоге оптимизация не имеет смысла, новых аллокаций тут и так не было бы // We avoid allocating anything.
Предположу, что это не единственный случай, я прошелся лишь поверхностно
Собственно очевидный вопрос - проверялись ли нововведения на вшивость путём бенчмарков?
Помимо этого я бы задал пару вопросов по тому, зачем было менять архитектуру выделяя отдельные методы но при этом не улучшать общую же логику, как к примеру с методом spreadFlag, который можно было БЫ заменить на switch вместо if лесенки, но это уже мелочи.
Если исправить всякие мелочи и описанные кейсы то получится добротно, но сейчас я бы скорее не стал использовать этот форк...
