Skip to content
This repository has been archived by the owner on Mar 3, 2024. It is now read-only.

Солнышко Ксения, ИТМО ФИТиП, M3332, Expiration #300

Merged
merged 24 commits into from
Jan 9, 2024

Conversation

persehoney
Copy link
Contributor

No description provided.

@AlexeyShik AlexeyShik self-requested a review December 7, 2023 08:46
@AlexeyShik AlexeyShik self-assigned this Dec 7, 2023
Copy link
Contributor

@AlexeyShik AlexeyShik left a comment

Choose a reason for hiding this comment

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

Решение рабочее, есть ряд предложений по улучшению, пока 20 баллов поставлю.


import ru.vk.itmo.Entry;

public record Triple<Data>(Data key, Data value, Data expiration) implements Entry<Data> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Triple не самое удачное название для класса, потому что не информативно и не масштабируемо, вот завтра понадобится еще поле добавить и Triple уже не подойдет. Можно, например, EntryImpl, EntryFull, EntryExtended.

protected boolean skip(Triple<MemorySegment> memorySegmentEntry) {
if (memorySegmentEntry.expiration() != null) {
return memorySegmentEntry.value() == null
|| memorySegmentEntry.expiration().toArray(ValueLayout.JAVA_LONG_UNALIGNED)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Можно достать Long из MemorySegment при создании итератора, тогда не придется делать это на каждый вызов skip, что явно избыточно

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Не очень понимаю. На момент создания итератора для каждого Entry достать long из expiration и заранее отфильтровать?

Copy link
Contributor

Choose a reason for hiding this comment

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

Да, все норм, что-то я неправильно понял, когда первый раз читал

protected boolean skip(Triple<MemorySegment> memorySegmentEntry) {
if (memorySegmentEntry.expiration() != null) {
return memorySegmentEntry.value() == null
|| memorySegmentEntry.expiration().toArray(ValueLayout.JAVA_LONG_UNALIGNED)[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

И вместо toArray можно сделать get по оффсету, потому что это более легкая операция

import java.lang.foreign.MemorySegment;
import java.util.Iterator;

public class FilterIterator implements Iterator<Entry<MemorySegment>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Судя по коду, этот итератор ничего не фильтрует, можно тогда удалить

@@ -90,14 +101,14 @@ public void save(Iterable<Entry<MemorySegment>> iterable)
MemorySegment fileSegment = utils.mapFile(fileChannel, indexSize + dataSize, writeArena);

// index:
// |key0_Start|value0_Start|key1_Start|value1_Start|key2_Start|value2_Start|...
// |key0_Start|value0_Start|expiration0_Start|key1_Start|value1_Start|expiration1_Start|key2_Start|...
Copy link
Contributor

Choose a reason for hiding this comment

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

Зачем в индекс записывается expiration? Выглядит, что это избыточная информация, причем для индекса это существенно, ведь вместо 2 лонгов хранить 3 значит, что индекс на 50% больше места занимает. Так как это не пользовательские данные, а служебные, не хотелось бы увеличивать их размер.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Так мы храним expiration аналогично с value, там могут быть томбстоуны, которые надо скипать

Copy link
Contributor

Choose a reason for hiding this comment

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

Тут я имел в виду, что индекс по факту используется для бинпоиска, поэтому в него можно не писать offset-ы value и expiration. В таком случае и локальность данных лучше будет, то есть в процессе бинпоиска будет читаться меньше страниц с диска.
Ваше решение тоже хорошее, но есть нюанс, что при range get будут читаться записи как из индекс части sstable, так и из data части. Эти части будут попадать в разные страницы, так как страница ~4KB, а sstable обычно в MB измеряется. Такое непоследовательное чтение будет работать чуть дольше последовательного.
Но это больше не к бонусу относится, а к прошлым дз, на самом деле

return new State(config, new ConcurrentSkipListMap<>(comparator), storage, diskStorage);
}

public void flush() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

При flush и close можно не писать на диск ключи, у которых вышел ttl.

@AlexeyShik
Copy link
Contributor

Все хорошо, поставлю баллов до 4

@AlexeyShik AlexeyShik merged commit 913870a into polis-vk:main Jan 9, 2024
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants