Skip to content

Commit

Permalink
Revert lazy build hashtable
Browse files Browse the repository at this point in the history 
This reverts
1) commit ad05dcb.
2) commit 23de11f.

PR #11791 (commit 23de11f and ad05dcb), which lazily builds the
hashtables for maps, introduced a regression for the case where the
MapBlock is created through AbstractMapBlock.getRegion(). The hashtables
built on the MapBlock region were not updated in the original MapBlock,
thus causing hashtables repeatedly being built on the same base MapBlock.
  • Loading branch information
@yingsu00
Ying Su authored and yingsu00 committed Jan 9, 2019
1 parent 8412a1f commit 79f480b
Show file tree
Hide file tree
Showing 8 changed files with 130 additions and 318 deletions.
29 changes: 0 additions & 29 deletions presto-main/src/test/java/com/facebook/presto/block/TestMapBlock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import org.testng.annotations.Test;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -81,34 +80,6 @@ public void testCompactBlock()
testIncompactBlock(mapType(TINYINT, TINYINT).createBlockFromKeyValue(Optional.of(mapIsNull), offsets, inCompactKeyBlock, inCompactValueBlock));
}

// TODO: remove this test when we have a more unified testWith() using assertBlock()
@Test
public void testLazyHashTableBuildOverBlockRegion()
{
Map<String, Long>[] values = createTestMap(9, 3, 4, 0, 8, 0, 6, 5);
Block block = createBlockWithValuesFromKeyValueBlock(values);
BlockBuilder blockBuilder = createBlockBuilderWithValues(values);

// Create a MapBlock that is a region of another MapBlock. It doesn't have hashtables built at the time of creation.
int offset = block.getPositionCount() / 2;
Block blockRegion = block.getRegion(offset, block.getPositionCount() - offset);

// Lazily build the hashtables for the block region and use them to do position/value check.
Map<String, Long>[] expectedValues = Arrays.copyOfRange(values, values.length / 2, values.length);
assertBlock(blockRegion, () -> blockBuilder.newBlockBuilderLike(null), expectedValues);

Map<String, Long>[] valuesWithNull = alternatingNullValues(values);
Block blockWithNull = createBlockWithValuesFromKeyValueBlock(valuesWithNull);

// Create a MapBlock that is a region of another MapBlock with null values. It doesn't have hashtables built at the time of creation.
offset = blockWithNull.getPositionCount() / 2;
Block blockRegionWithNull = blockWithNull.getRegion(offset, blockWithNull.getPositionCount() - offset);

// Lazily build the hashtables for the block region and use them to do position/value check.
Map<String, Long>[] expectedValuesWithNull = Arrays.copyOfRange(valuesWithNull, valuesWithNull.length / 2, valuesWithNull.length);
assertBlock(blockRegionWithNull, () -> blockBuilder.newBlockBuilderLike(null), expectedValuesWithNull);
}

private Map<String, Long>[] createTestMap(int... entryCounts)
{
Map<String, Long>[] result = new Map[entryCounts.length];
Expand Down
77 changes: 25 additions & 52 deletions presto-spi/src/main/java/com/facebook/presto/spi/block/AbstractMapBlock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,23 +39,20 @@ public abstract class AbstractMapBlock
protected final Type keyType;
protected final MethodHandle keyNativeHashCode;
protected final MethodHandle keyBlockNativeEquals;
protected final MethodHandle keyBlockHashCode;

public AbstractMapBlock(Type keyType, MethodHandle keyNativeHashCode, MethodHandle keyBlockNativeEquals, MethodHandle keyBlockHashCode)
public AbstractMapBlock(Type keyType, MethodHandle keyNativeHashCode, MethodHandle keyBlockNativeEquals)
{
this.keyType = requireNonNull(keyType, "keyType is null");
// keyNativeHashCode can only be null due to map block kill switch. deprecated.new-map-block
this.keyNativeHashCode = keyNativeHashCode;
// keyBlockNativeEquals can only be null due to map block kill switch. deprecated.new-map-block
this.keyBlockNativeEquals = keyBlockNativeEquals;
this.keyBlockHashCode = requireNonNull(keyBlockHashCode, "keyBlockHashCode is null");
}

protected abstract Block getRawKeyBlock();

protected abstract Block getRawValueBlock();

@Nullable
protected abstract int[] getHashTables();

/**
Expand All @@ -73,8 +70,6 @@ public AbstractMapBlock(Type keyType, MethodHandle keyNativeHashCode, MethodHand
@Nullable
protected abstract boolean[] getMapIsNull();

protected abstract void ensureHashTableLoaded();

int getOffset(int position)
{
return getOffsets()[position + getOffsetBase()];
Expand Down Expand Up @@ -117,35 +112,21 @@ public Block copyPositions(int[] positions, int offset, int length)
}

int[] hashTable = getHashTables();
int[] newHashTable = null;
if (hashTable != null) {
newHashTable = new int[newOffsets[newOffsets.length - 1] * HASH_MULTIPLIER];
int newHashIndex = 0;
for (int i = offset; i < offset + length; ++i) {
int position = positions[i];
int entriesStartOffset = getOffset(position);
int entriesEndOffset = getOffset(position + 1);
for (int hashIndex = entriesStartOffset * HASH_MULTIPLIER; hashIndex < entriesEndOffset * HASH_MULTIPLIER; hashIndex++) {
newHashTable[newHashIndex] = hashTable[hashIndex];
newHashIndex++;
}
int[] newHashTable = new int[newOffsets[newOffsets.length - 1] * HASH_MULTIPLIER];
int newHashIndex = 0;
for (int i = offset; i < offset + length; ++i) {
int position = positions[i];
int entriesStartOffset = getOffset(position);
int entriesEndOffset = getOffset(position + 1);
for (int hashIndex = entriesStartOffset * HASH_MULTIPLIER; hashIndex < entriesEndOffset * HASH_MULTIPLIER; hashIndex++) {
newHashTable[newHashIndex] = hashTable[hashIndex];
newHashIndex++;
}
}

Block newKeys = getRawKeyBlock().copyPositions(entriesPositions.elements(), 0, entriesPositions.size());
Block newValues = getRawValueBlock().copyPositions(entriesPositions.elements(), 0, entriesPositions.size());
return createMapBlockInternal(
0,
length,
Optional.of(newMapIsNull),
newOffsets,
newKeys,
newValues,
Optional.ofNullable(newHashTable),
keyType,
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
return createMapBlockInternal(0, length, Optional.of(newMapIsNull), newOffsets, newKeys, newValues, newHashTable, keyType, keyBlockNativeEquals, keyNativeHashCode);
}

@Override
Expand All @@ -161,11 +142,10 @@ public Block getRegion(int position, int length)
getOffsets(),
getRawKeyBlock(),
getRawValueBlock(),
Optional.ofNullable(getHashTables()),
getHashTables(),
keyType,
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
keyNativeHashCode);
}

@Override
Expand Down Expand Up @@ -227,12 +207,7 @@ public Block copyRegion(int position, int length)
int[] newOffsets = compactOffsets(getOffsets(), position + getOffsetBase(), length);
boolean[] mapIsNull = getMapIsNull();
boolean[] newMapIsNull = mapIsNull == null ? null : compactArray(mapIsNull, position + getOffsetBase(), length);

int[] hashTables = getHashTables();
int[] newHashTable = null;
if (hashTables != null) {
newHashTable = compactArray(hashTables, startValueOffset * HASH_MULTIPLIER, (endValueOffset - startValueOffset) * HASH_MULTIPLIER);
}
int[] newHashTable = compactArray(getHashTables(), startValueOffset * HASH_MULTIPLIER, (endValueOffset - startValueOffset) * HASH_MULTIPLIER);

if (newKeys == getRawKeyBlock() && newValues == getRawValueBlock() && newOffsets == getOffsets() && newMapIsNull == mapIsNull && newHashTable == getHashTables()) {
return this;
Expand All @@ -244,11 +219,10 @@ public Block copyRegion(int position, int length)
newOffsets,
newKeys,
newValues,
Optional.ofNullable(newHashTable),
newHashTable,
keyType,
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
keyNativeHashCode);
}

@Override
Expand All @@ -264,7 +238,12 @@ public <T> T getObject(int position, Class<T> clazz)
return clazz.cast(new SingleMapBlock(
startEntryOffset * 2,
(endEntryOffset - startEntryOffset) * 2,
this));
getRawKeyBlock(),
getRawValueBlock(),
getHashTables(),
keyType,
keyNativeHashCode,
keyBlockNativeEquals));
}

@Override
Expand All @@ -284,12 +263,7 @@ public Block getSingleValueBlock(int position)
int valueLength = endValueOffset - startValueOffset;
Block newKeys = getRawKeyBlock().copyRegion(startValueOffset, valueLength);
Block newValues = getRawValueBlock().copyRegion(startValueOffset, valueLength);

int[] hashTables = getHashTables();
int[] newHashTable = null;
if (hashTables != null) {
newHashTable = Arrays.copyOfRange(hashTables, startValueOffset * HASH_MULTIPLIER, endValueOffset * HASH_MULTIPLIER);
}
int[] newHashTable = Arrays.copyOfRange(getHashTables(), startValueOffset * HASH_MULTIPLIER, endValueOffset * HASH_MULTIPLIER);

return createMapBlockInternal(
0,
Expand All @@ -298,11 +272,10 @@ public Block getSingleValueBlock(int position)
new int[] {0, valueLength},
newKeys,
newValues,
Optional.ofNullable(newHashTable),
newHashTable,
keyType,
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
keyNativeHashCode);
}

@Override
Expand Down
103 changes: 28 additions & 75 deletions presto-spi/src/main/java/com/facebook/presto/spi/block/MapBlock.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@

import static com.facebook.presto.spi.block.MapBlockBuilder.buildHashTable;
import static io.airlift.slice.SizeOf.sizeOf;
import static io.airlift.slice.SizeOf.sizeOfIntArray;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

Expand All @@ -43,7 +42,7 @@ public class MapBlock
private final int[] offsets;
private final Block keyBlock;
private final Block valueBlock;
private volatile int[] hashTables; // hash to location in map. Writes to the field is protected by "this" monitor.
private final int[] hashTables; // hash to location in map;

private volatile long sizeInBytes;
private final long retainedSizeInBytes;
Expand All @@ -69,6 +68,20 @@ public static MapBlock fromKeyValueBlock(
validateConstructorArguments(0, offsets.length - 1, mapIsNull.orElse(null), offsets, keyBlock, valueBlock, mapType.getKeyType(), keyBlockNativeEquals, keyNativeHashCode);

int mapCount = offsets.length - 1;
int elementCount = keyBlock.getPositionCount();
int[] hashTables = new int[elementCount * HASH_MULTIPLIER];
Arrays.fill(hashTables, -1);
for (int i = 0; i < mapCount; i++) {
int keyOffset = offsets[i];
int keyCount = offsets[i + 1] - keyOffset;
if (keyCount < 0) {
throw new IllegalArgumentException(format("Offset is not monotonically ascending. offsets[%s]=%s, offsets[%s]=%s", i, offsets[i], i + 1, offsets[i + 1]));
}
if (mapIsNull.isPresent() && mapIsNull.get()[i] && keyCount != 0) {
throw new IllegalArgumentException("A null map must have zero entries");
}
buildHashTable(keyBlock, keyOffset, keyCount, keyBlockHashCode, hashTables, keyOffset * HASH_MULTIPLIER, keyCount * HASH_MULTIPLIER);
}

return createMapBlockInternal(
0,
Expand All @@ -77,11 +90,10 @@ public static MapBlock fromKeyValueBlock(
offsets,
keyBlock,
valueBlock,
Optional.empty(),
hashTables,
mapType.getKeyType(),
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
keyNativeHashCode);
}

/**
Expand All @@ -100,25 +112,13 @@ public static MapBlock createMapBlockInternal(
int[] offsets,
Block keyBlock,
Block valueBlock,
Optional<int[]> hashTables,
int[] hashTables,
Type keyType,
MethodHandle keyBlockNativeEquals,
MethodHandle keyNativeHashCode,
MethodHandle keyBlockHashCode)
MethodHandle keyNativeHashCode)
{
validateConstructorArguments(startOffset, positionCount, mapIsNull.orElse(null), offsets, keyBlock, valueBlock, keyType, keyBlockNativeEquals, keyNativeHashCode);
return new MapBlock(
startOffset,
positionCount,
mapIsNull.orElse(null),
offsets,
keyBlock,
valueBlock,
hashTables.orElse(null),
keyType,
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
return new MapBlock(startOffset, positionCount, mapIsNull.orElse(null), offsets, keyBlock, valueBlock, hashTables, keyType, keyBlockNativeEquals, keyNativeHashCode);
}

private static void validateConstructorArguments(
Expand Down Expand Up @@ -171,15 +171,15 @@ private MapBlock(
int[] offsets,
Block keyBlock,
Block valueBlock,
@Nullable int[] hashTables,
int[] hashTables,
Type keyType,
MethodHandle keyBlockNativeEquals,
MethodHandle keyNativeHashCode,
MethodHandle keyBlockHashCode)
MethodHandle keyNativeHashCode)
{
super(keyType, keyNativeHashCode, keyBlockNativeEquals, keyBlockHashCode);
super(keyType, keyNativeHashCode, keyBlockNativeEquals);

if (hashTables != null && hashTables.length < keyBlock.getPositionCount() * HASH_MULTIPLIER) {
requireNonNull(hashTables, "hashTables is null");
if (hashTables.length < keyBlock.getPositionCount() * HASH_MULTIPLIER) {
throw new IllegalArgumentException(format("keyBlock/valueBlock size does not match hash table size: %s %s", keyBlock.getPositionCount(), hashTables.length));
}

Expand All @@ -192,16 +192,7 @@ private MapBlock(
this.hashTables = hashTables;

this.sizeInBytes = -1;

// We will add the hashtable size to the retained size even if it's not built yet. This could be overestimating
// but is necessary to avoid reliability issues. Currently the memory counting framework only pull the retained
// size once for each operator so updating in the middle of the processing would not work.
this.retainedSizeInBytes = INSTANCE_SIZE
+ keyBlock.getRetainedSizeInBytes()
+ valueBlock.getRetainedSizeInBytes()
+ sizeOf(offsets)
+ sizeOf(mapIsNull)
+ sizeOfIntArray(keyBlock.getPositionCount() * HASH_MULTIPLIER); // hashtable size if it was built
this.retainedSizeInBytes = INSTANCE_SIZE + keyBlock.getRetainedSizeInBytes() + valueBlock.getRetainedSizeInBytes() + sizeOf(offsets) + sizeOf(mapIsNull) + sizeOf(hashTables);
}

@Override
Expand Down Expand Up @@ -312,47 +303,9 @@ public Block getLoadedBlock()
offsets,
keyBlock,
loadedValueBlock,
Optional.ofNullable(hashTables),
hashTables,
keyType,
keyBlockNativeEquals,
keyNativeHashCode,
keyBlockHashCode);
}

@Override
protected void ensureHashTableLoaded()
{
if (this.hashTables != null) {
return;
}

// This can only happen for MapBlock, not MapBlockBuilder because the latter always has non-null hashtables
synchronized (this) {
if (this.hashTables != null) {
return;
}

int[] hashTables = new int[getRawKeyBlock().getPositionCount() * HASH_MULTIPLIER];
Arrays.fill(hashTables, -1);
for (int i = 0; i < offsets.length - 1; i++) {
int keyOffset = offsets[i];
int keyCount = offsets[i + 1] - keyOffset;
if (keyCount < 0) {
throw new IllegalArgumentException(format("Offset is not monotonically ascending. offsets[%s]=%s, offsets[%s]=%s", i, offsets[i], i + 1, offsets[i + 1]));
}
if (mapIsNull != null && mapIsNull[i] && keyCount != 0) {
throw new IllegalArgumentException("A null map must have zero entries");
}
buildHashTable(
getRawKeyBlock(),
keyOffset,
keyCount,
keyBlockHashCode,
hashTables,
keyOffset * HASH_MULTIPLIER,
keyCount * HASH_MULTIPLIER);
}
this.hashTables = hashTables;
}
keyNativeHashCode);
}
}
Loading

0 comments on commit 79f480b

Please sign in to comment.