-
Notifications
You must be signed in to change notification settings - Fork 77
Коротких Виктор / ИТМО DWS / Stage 6 / Compression #304
Conversation
@incubos Добрый день! Можете, пожалуйста, посмотреть моё решение 6го этапа, как будет время? |
…or with compression disabled
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ревьюил бегло. Есть некоторые замечания. Хороший дизайн и набор тестов, хотя всегда есть, что улучшать. Интересное решение с кеширование разжатого блока. Не хватает бенчмарков: время чтения/флаша с компрессией vs выигрыш по месту на диске -- но там многое зависит от набора данных. Тем не менее, предлагаю на этом остановиться.
50 баллов за бонусную фичу.
@Override | ||
public byte[] compress(byte[] src, int len) throws IOException { | ||
byte[] dst = new byte[len]; | ||
long originalSize = Zstd.compressByteArray( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Что будет, если не влезет в dst
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
По идее в самых вырожденных случаях сжатые данные будут по размеру равны несжатым.
Но вы правы - тут лучше было бы сделать так:
byte[] dst = new byte[(int) Zstd.compressBound(len)];
Тогда мы будем аллоцировать больше памяти, но зато будем уверены, что 100% влезем в массив
if (from != null) { | ||
fromPosition = getEntryOffset(from, SearchOption.GTE); | ||
if (fromPosition == -1) { | ||
return new BaseSSTableIterator(0, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно возвращать single empty iterator и не аллоцировать, верно?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, не учёл этот момент. Но надо тогда создать имплементацию LSMPointerIterator:
public class EmptyLSMPointerIterator extends LSMPointerIterator {
...
}
if (to != null) { | ||
toPosition = getEntryOffset(to, SearchOption.LT); | ||
if (toPosition == -1) { | ||
return new BaseSSTableIterator(0, -1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Можно возвращать single empty iterator и не аллоцировать, верно?
long fromPosition = getMinKeySizeOffset(); | ||
long toPosition = getMaxKeySizeOffset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Видимо, эта инициализация должна быть в ветках else
ниже? Reassignment подобных переменных усложняет понимание кода.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь подразумевалось, что мы изначально инициализируем эти переменные границами всей sstable, а потом если у нас заданы границы from и to, то переназначаем новыми значениями.
Соглашусь, что наверное через else ветки было бы понятнее
long keySize = mappedSSTable.get(ValueLayout.JAVA_LONG_UNALIGNED, fromPosition); | ||
long valueOffset = fromPosition + Long.BYTES + keySize; | ||
long valueSize = mappedSSTable.get(ValueLayout.JAVA_LONG_UNALIGNED, valueOffset); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Этот код дублируется в нескольких местах. Отрефачить бы...
// ZSTD specific | ||
if (decompressor instanceof ZstdDecompressor) return uncompressedBlockSize; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Текут абстракции :/
for (int i = Long.BYTES - 1; i >= 0; i--) { | ||
value <<= 8; | ||
value |= (bytes[i] & 0xFFL); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не факт, что соптимизируется. Надёжнее по старинке без цикла собрать long
из восьми байт.
*/ | ||
public void write( | ||
boolean isCompacted, | ||
Supplier<? extends Iterator<? extends Entry<MemorySegment>>> iteratorSupplier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем здесь Supplier
, если get()
на нём вызывается ровно один раз?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Остатки старого кода, в котором несколько раз пробегались по итератору
while (entries.hasNext()) { | ||
// Then write the entry | ||
final Entry<MemorySegment> entry = entries.next(); | ||
hasNoTombstones = entry.value() != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Значение может многократно меняться между true
и false
, верно?
hasNoTombstones = entry.value() != null; | |
hasNoTombstones &= entry.value() != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, тут сам себя запутал. Ваше решение более корректное
@@ -54,7 +54,7 @@ private static String duration(int seconds) { | |||
} | |||
|
|||
@DaoTest(stage = 3) | |||
void database(Dao<String, Entry<String>> dao) throws Exception { | |||
public void database(Dao<String, Entry<String>> dao) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем это?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тестировал сжатие данных с помощью реализаций существующих тестов, а так как пакеты разные, то пришлось сделать public.
Не смог придумать ничего лучше, чтобы при этом не копировать код тестов. В целом можно было бы попробовать по умолчанию в тестах использовать конфиг с включённым сжатием (затронуло бы только мою реализацию), но тогда есть шанс, что тесты по таймаутам будут падать, потому что сжатие/разжатие будет съедать время (и памяти из-за буфферов будет требоваться больше)
Спасибо за ответы. |
Реализация сжатия данных, хранящихся на диске в sstable.
Записывается 3 файла:
keyNBlockNumber - номер блока для начала ключа номер N (key1Size|key1|value1Size|value1)
keyNSizeBlockOffset - смещение начала размера ключа внутри блока
Запись ведётся с помощью реализации AbstractSSTableWriter:
Чтение ведётся с помощью реализации AbstractSSTableReader:
CompressedSSTableReader также использует буфер для расжатых данных и сохраняет информацию о последнем прочитанном блоке, чтобы если необходимо будет прочитать данные из того же блока, то не приходилось заново читать и разжимать данные. Особенно полезно для итератора.
Чтение данных происходит следующим образом:
Также сжатие можно настраивать через конфиг при открытии дао
По сути изменение конфига при переоткрытии будет влиять на новые флашнутые файлы и на результат compaction.
Алгоритмы сжатия: