Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Плясов Климентий, ИТМО DWS, stage 6 #228

Closed
wants to merge 45 commits into from

Conversation

Handiesto
Copy link
Contributor

No description provided.

@incubos
Copy link
Member

incubos commented Apr 28, 2024

PR открыт после soft deadline, поэтому максимум 5 баллов.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 15917 lines exceeds the maximum allowed for the inline comments feature.

@incubos incubos requested review from incubos and removed request for vladimir-bf April 29, 2024 15:58
@incubos incubos assigned incubos and unassigned vladimir-bf Apr 29, 2024
Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 16914 lines exceeds the maximum allowed for the inline comments feature.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 16933 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Member

@incubos incubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Стриминг получился жадным, а не ленивым. Есть вопросы к реализации и результатам профилирования.

if ("/v0/entities".equals(path)) {
handleEntitiesRequest(request, session);
} else {
handleOtherRequest(request, session);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не очень удачное имя метода.

Comment on lines 378 to 380
while (iterator.hasNext()) {
ChunkGenerator.writeDataChunk(session, iterator.next());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

В случае медленного клиента мы затянем все данные в память и упадём по OOM, верно? Вся суть задания в том, чтобы этого избежать. Будете переделывать?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Немного не пониманию, я же отправляю в сессию данные, ничего не оставляя в памяти. Подскажите, пожалуйста, что не так?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Данные не отправляются сразу, с ставятся в очередь на отправку -- см. Session.write(). Но в очередь добавляется всё содержимое Iterator, т.е. никакого streaming нет.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Понял, спасибо

### one/nio/net/Session.write
занимает 46% всей выделенной памяти. Внутри этого метода распределение памяти выглядит следующим образом:

- writeDataChunk - 70%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Откуда на профиле аллокаций Class.getSimpleName() с почти 20%?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private <Z> Z toArray(Class<Z> arrayClass, ValueLayout elemLayout, IntFunction<Z> arrayFactory, Function<Z, MemorySegment> segmentFactory) {
        int size = checkArraySize(arrayClass.getSimpleName(), (int)elemLayout.byteSize());
        Z arr = arrayFactory.apply(size);
        MemorySegment arrSegment = segmentFactory.apply(arr);
        MemorySegment.copy(this, elemLayout, 0, arrSegment, elemLayout.withOrder(ByteOrder.nativeOrder()), 0, size);
        return arr;
    }

когда мы вызываем toArray в MemorySegment, он вызывает этот метод, чтобы проверить размер массива

### one/nio/net/Session.write
занимает 46% всей выделенной памяти. Внутри этого метода распределение памяти выглядит следующим образом:

- writeDataChunk - 70%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Откуда аллокация HeapByteBuffer? Можно её избежать?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Насколько я понимаю он начинает использоваться в SocketChannel.write, где на вход идет bytebuffer и там где-то внутри обрабатывается.

Copy link

@codeclimate codeclimate bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR diff size of 16933 lines exceeds the maximum allowed for the inline comments feature.

Copy link
Member

@incubos incubos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Функционально реализовано неверно, потому что отправка жадная. Будете исправлять? Напоминаю, что максимум 5 баллов, потому что после soft deadline.

@Handiesto
Copy link
Contributor Author

@incubos Не буду. К сожалению, мне уже не хватает баллов до 50(

@incubos
Copy link
Member

incubos commented May 18, 2024

Тогда 1 балл за попытку :(

@incubos incubos closed this May 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants