Свистухин Андрей, ИТМО DWS, Stage 5 #186
Conversation
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
|
Проблему с аллокациями пофиксил в следующей лабораторной. Если нужно, могу изменения подлить сюда. |
| Request request, | ||
| List<String> nodeUrls, | ||
| int ack, | ||
| List<CompletableFuture<Response>> futures, |
There was a problem hiding this comment.
Очень плохой паттерн: передавать в параметрах IN и OUT переменные. Лучше стараться так организовать код, чтобы в пармтерах были только IN, а OUT было в return.
И лучше чтобы это было что-то одно
There was a problem hiding this comment.
Нужно было эти futures возвращать return'ом и дальше записывать их в структуру которая лежит по стеку вызовов выше? Я просто подумал, что может быть из-за этого у меня и сходил с ума GC. Но кажется, проблема была всё-таки не в этом
There was a problem hiding this comment.
да, я бы сделал так, хотя понял что вы пробовали уменьшить число аллокаций
| responses.put(url, response); | ||
| } | ||
| if (unsuccessfulResponsesCount.get() >= ack) { | ||
| futures.add(CompletableFuture.completedFuture(null)); |
There was a problem hiding this comment.
тут наверное планировалось поставить return?
There was a problem hiding this comment.
если учитывать Ваш коммент выше, то получается так))
| CompletableFuture<Response> futureResponse = proxyRequest(request, url); | ||
| CompletableFuture<Response> resultFuture = futureResponse.thenApply(response -> { | ||
| boolean success = ServerImpl.isSuccessProcessed(response.getStatus()); | ||
| if (success && responsesCollected.getAndIncrement() < ack) { |
There was a problem hiding this comment.
тут что-то не то, на SUCCESS ответ, который будет ack+1 мы почему-то взведем unsuccessfulResponsesCount
There was a problem hiding this comment.
Да, проглядел этот момент. Исправлю.
| long timestamp = httpResponse.headers().firstValueAsLong(RequestWrapper.NIO_TIMESTAMP_HEADER).orElse(0); | ||
| long timestamp = Long.parseLong( | ||
| httpResponse.headers() | ||
| .firstValue(RequestWrapper.NIO_TIMESTAMP_HEADER) |
There was a problem hiding this comment.
а эта бесполезная штука так и переехала дальше в этот стейдж?
There was a problem hiding this comment.
Нет, она не бесполезная. В ServerImpl идет проверка, есть ли этот хедер или нет
String timestamp = response.getHeader(RequestWrapper.NIO_TIMESTAMP_STRING_HEADER);
| List<Response> collectedResponses, | ||
| AtomicInteger unsuccessfulResponsesCount | ||
| ) { | ||
| futures.add(new CompletableFuture<Response>().completeAsync(() -> { |
There was a problem hiding this comment.
а тут мы будем использовать commonPool, что плохо, лучше пользоваться своим пулом
There was a problem hiding this comment.
А нам ведь нужно создать отдельный пул под это, а не использовать пул из ServerImpl?
| съедает сетевое взаимодействие нод, из-за того, что мы теперь не блокируемся на запросе к серверу. | ||
| В остальном всё выглядит также, почти всё процессорное время съедает сетевое взаимодействие. | ||
| #### ALLOC | ||
| Аллокации убили GC. ~97% аллоков занимает GC, а именно AllocateHeap. Видимо, я не совсем правильно использую CompletableFuture, |
There was a problem hiding this comment.
ну вообще для этого и есть инструмент, хорошо бы найти проблему и починить
There was a problem hiding this comment.
Так как тест проходил локально, я подумал что проблема флаков либо у кого-нибудь другого, либо агент подвисает. И только уже при профилировании заметил, что проблема всё-таки у меня. В шестом этапе я пытался исправить логику с CompletableFutures, думал, что они у меня подвисают в памяти, но кажется это не помогло. Тесты на агенте там прошли, и я думал что проблема тоже ушла, ведь профилирование ещё не делал на том этапе. Вот сегодня как сделал профилирование, снова увидел, что проблема никуда не ушла. Пока что не разобрался с чем именно проблема, ведь CompletableFutures, вроде как, попытался исправить.
И странно, ведь именно после этого этапа у меня GC начал занимать все аллокации на JVM. Возможно, имеет смысл проверить через visualvm, посмотреть что в рантайме находится в памяти.
|
Они не проходят на агенте скорее всего из-за большого потребления памяти. Локально все тесты проходят. |
|
@atimofeyev исправил код по комментариям |
|
После исправления ошибки с тесты начали проходить, я решил проверить профилировщик, подумал что может быть отсюда и утечка, но нет, GC до сих пор ест аллоки |
| return new CompletableFuture<Response>().completeAsync(() -> { | ||
| Response response = handle(request); | ||
| if (ServerImpl.isSuccessProcessed(response.getStatus())) { | ||
| collectedResponses.add(response); |
There was a problem hiding this comment.
здесь идет незащищенное обращение к коллекции из разных потоков, должны быть ошибки
There was a problem hiding this comment.
а, и правда
я почему-то подумал, что можно и обычный arraylist сделать, так как у нас для каждого запроса будет создаваться свой лист, но проблема будет именно из-за того, что в разных потоках добавляем туда
изначально у меня и было на скиплисте, но что-то я решил поменять на arraylist
There was a problem hiding this comment.
А, нет, я на ArrayList не менял
Я передаю в метод
List<Response> validResponses = new CopyOnWriteArrayList<>();
There was a problem hiding this comment.
хорошо, значит я проглядел
| CompletableFuture<Response> resultFuture = futureResponse.thenApply(response -> { | ||
| boolean success = ServerImpl.isSuccessProcessed(response.getStatus()); | ||
| if (success && responsesCollected.getAndIncrement() < ack) { | ||
| collectedResponses.add(response); |
There was a problem hiding this comment.
здесь идет незащищенное обращение к коллекции из разных потоков, должны быть ошибки
No description provided.