sashapolo commented on code in PR #6685:
URL: https://github.com/apache/ignite-3/pull/6685#discussion_r2768267126
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryStorageEngine.java:
##########
@@ -17,26 +17,173 @@
package org.apache.ignite.internal.storage.pagememory;
+import static java.util.Comparator.comparing;
+import static
org.apache.ignite.internal.util.IgniteUtils.lexicographicListComparator;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation;
+import org.apache.ignite.internal.configuration.SystemLocalConfiguration;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
import org.apache.ignite.internal.hlc.HybridClock;
import org.apache.ignite.internal.pagememory.PageMemory;
import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.storage.StorageException;
import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor;
+import
org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor.StorageSortedIndexColumnDescriptor;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorGenerator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorOptions;
+import org.apache.ignite.internal.type.NativeType;
+import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
/** Abstract implementation of the storage engine based on memory {@link
PageMemory}. */
public abstract class AbstractPageMemoryStorageEngine implements StorageEngine
{
+ public static final String
LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
+
+ protected final @Nullable SystemLocalConfiguration systemLocalConfig;
+
private final HybridClock clock;
+ private boolean useLegacySortedIndexComparator = false;
+
+ /**
+ * This map is used to reuse comparators for sorted indexes with the same
set of columns and their collations. It is beneficial to reuse
+ * comparators because otherwise every comparator will use its own
generated class, which bloats metaspace and doesn't allow JVM's JIT
+ * to be as efficient.
+ */
+ private final ConcurrentMap<StorageSortedIndexDescriptor,
CachedComparator> cachedSortedIndexComparators
+ = new ConcurrentSkipListMap<>(comparing(
Review Comment:
Why do you need a `ConcurrentSkipListMap` here and not a `ConcurrentHashMap`?
##########
check-rules/spotbugs-excludes.xml:
##########
@@ -278,6 +278,12 @@
<Class name="org.apache.ignite.internal.util.OffheapReadWriteLock"/>
<Method name="awaitCondition"/>
</Match>
+ <Match>
+ <!-- Done that way on purpose. -->
Review Comment:
A better explanation is needed here
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryStorageEngine.java:
##########
@@ -17,26 +17,173 @@
package org.apache.ignite.internal.storage.pagememory;
+import static java.util.Comparator.comparing;
+import static
org.apache.ignite.internal.util.IgniteUtils.lexicographicListComparator;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation;
+import org.apache.ignite.internal.configuration.SystemLocalConfiguration;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
import org.apache.ignite.internal.hlc.HybridClock;
import org.apache.ignite.internal.pagememory.PageMemory;
import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.storage.StorageException;
import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor;
+import
org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor.StorageSortedIndexColumnDescriptor;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorGenerator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorOptions;
+import org.apache.ignite.internal.type.NativeType;
+import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
/** Abstract implementation of the storage engine based on memory {@link
PageMemory}. */
public abstract class AbstractPageMemoryStorageEngine implements StorageEngine
{
+ public static final String
LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
Review Comment:
```suggestion
public static final String
LEGACY_PAGE_MEMORY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
```
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryStorageEngine.java:
##########
@@ -17,26 +17,173 @@
package org.apache.ignite.internal.storage.pagememory;
+import static java.util.Comparator.comparing;
+import static
org.apache.ignite.internal.util.IgniteUtils.lexicographicListComparator;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation;
+import org.apache.ignite.internal.configuration.SystemLocalConfiguration;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
import org.apache.ignite.internal.hlc.HybridClock;
import org.apache.ignite.internal.pagememory.PageMemory;
import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.storage.StorageException;
import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor;
+import
org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor.StorageSortedIndexColumnDescriptor;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorGenerator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorOptions;
+import org.apache.ignite.internal.type.NativeType;
+import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
/** Abstract implementation of the storage engine based on memory {@link
PageMemory}. */
public abstract class AbstractPageMemoryStorageEngine implements StorageEngine
{
+ public static final String
LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
+
+ protected final @Nullable SystemLocalConfiguration systemLocalConfig;
Review Comment:
Why is it `Nullable`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/index/sorted/SortedIndexRowKey.java:
##########
@@ -33,10 +34,18 @@ public class SortedIndexRowKey implements IndexRowKey {
*/
SortedIndexRowKey(IndexColumns indexColumns) {
this.indexColumns = indexColumns;
+ this.myAccessor = new
UnsafeByteBufferAccessor(indexColumns.valueBuffer());
+ this.otherAccessor = new UnsafeByteBufferAccessor(0, 0);
}
@Override
public IndexColumns indexColumns() {
return indexColumns;
}
+
+ /** Cached accessor instance for the {@link #indexColumns}. Used to avoid
constant reallocations. */
+ public final UnsafeByteBufferAccessor myAccessor;
+
+ /** Cached accessor instance for the arbitrary buffer. Used to avoid
reallocations. */
Review Comment:
It's weird that it is not used to avoid **constant** reallocations =)
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryStorageEngine.java:
##########
@@ -17,26 +17,173 @@
package org.apache.ignite.internal.storage.pagememory;
+import static java.util.Comparator.comparing;
+import static
org.apache.ignite.internal.util.IgniteUtils.lexicographicListComparator;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation;
+import org.apache.ignite.internal.configuration.SystemLocalConfiguration;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
import org.apache.ignite.internal.hlc.HybridClock;
import org.apache.ignite.internal.pagememory.PageMemory;
import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.storage.StorageException;
import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor;
+import
org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor.StorageSortedIndexColumnDescriptor;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorGenerator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorOptions;
+import org.apache.ignite.internal.type.NativeType;
+import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
/** Abstract implementation of the storage engine based on memory {@link
PageMemory}. */
public abstract class AbstractPageMemoryStorageEngine implements StorageEngine
{
+ public static final String
LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
+
+ protected final @Nullable SystemLocalConfiguration systemLocalConfig;
+
private final HybridClock clock;
+ private boolean useLegacySortedIndexComparator = false;
Review Comment:
Does it need to be `volatile`?
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryStorageEngine.java:
##########
@@ -17,26 +17,173 @@
package org.apache.ignite.internal.storage.pagememory;
+import static java.util.Comparator.comparing;
+import static
org.apache.ignite.internal.util.IgniteUtils.lexicographicListComparator;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation;
+import org.apache.ignite.internal.configuration.SystemLocalConfiguration;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
import org.apache.ignite.internal.hlc.HybridClock;
import org.apache.ignite.internal.pagememory.PageMemory;
import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.storage.StorageException;
import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor;
+import
org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor.StorageSortedIndexColumnDescriptor;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorGenerator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorOptions;
+import org.apache.ignite.internal.type.NativeType;
+import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
/** Abstract implementation of the storage engine based on memory {@link
PageMemory}. */
public abstract class AbstractPageMemoryStorageEngine implements StorageEngine
{
+ public static final String
LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
+
+ protected final @Nullable SystemLocalConfiguration systemLocalConfig;
+
private final HybridClock clock;
+ private boolean useLegacySortedIndexComparator = false;
+
+ /**
+ * This map is used to reuse comparators for sorted indexes with the same
set of columns and their collations. It is beneficial to reuse
+ * comparators because otherwise every comparator will use its own
generated class, which bloats metaspace and doesn't allow JVM's JIT
+ * to be as efficient.
+ */
+ private final ConcurrentMap<StorageSortedIndexDescriptor,
CachedComparator> cachedSortedIndexComparators
+ = new ConcurrentSkipListMap<>(comparing(
+ StorageSortedIndexDescriptor::columns,
+
lexicographicListComparator(comparing(StorageSortedIndexColumnDescriptor::type)
+
.thenComparing(StorageSortedIndexColumnDescriptor::nullable)
+
.thenComparing(StorageSortedIndexColumnDescriptor::nullsFirst)
+
.thenComparing(StorageSortedIndexColumnDescriptor::asc)
+ )
+ ));
+
/** Constructor. */
- AbstractPageMemoryStorageEngine(HybridClock clock) {
+ AbstractPageMemoryStorageEngine(@Nullable SystemLocalConfiguration
systemLocalConfig, HybridClock clock) {
+ this.systemLocalConfig = systemLocalConfig;
this.clock = clock;
}
+ @Override
+ public void start() throws StorageException {
+ if (systemLocalConfig != null) {
+ SystemPropertyView legacyComparator =
systemLocalConfig.value().properties()
+ .get(LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY);
+
+ if (legacyComparator != null &&
"true".equalsIgnoreCase(legacyComparator.propertyValue())) {
+ useLegacySortedIndexComparator = true;
+ }
+ }
+ }
+
/**
* Creates a Global remove ID for structures based on a {@link BplusTree},
always creating monotonically increasing ones even after
* recovery node, so that there are no errors after restoring trees.
*/
public AtomicLong generateGlobalRemoveId() {
return new AtomicLong(clock.nowLong());
}
+
+ /**
+ * Creates a new instance of {@link JitComparator} for the given sorted
index descriptor.
+ */
+ @VisibleForTesting
+ public static JitComparator
createNewJitComparator(StorageSortedIndexDescriptor desc) {
+ List<StorageSortedIndexColumnDescriptor> columns = desc.columns();
+ List<CatalogColumnCollation> collations = new
ArrayList<>(columns.size());
+ List<NativeType> types = new ArrayList<>(columns.size());
+ List<Boolean> nullableFlags = new ArrayList<>(columns.size());
+
+ for (StorageSortedIndexColumnDescriptor col : columns) {
+ collations.add(CatalogColumnCollation.get(col.asc(),
col.nullsFirst()));
+ types.add(col.type());
+ // Nulls can still be passed from the outside as lower/upper
bounds during the search, even if the column is not nullable.
+ nullableFlags.add(true);
+ }
+
+ return
JitComparatorGenerator.createComparator(JitComparatorOptions.builder()
+ .columnCollations(collations)
+ .columnTypes(types)
+ .nullableFlags(nullableFlags)
+ .supportPrefixes(true)
+ .supportPartialComparison(true)
+ .build()
+ );
+ }
+
+ /**
+ * Creates or retrieves from cache a {@link JitComparator} for the given
sorted index descriptor. Returns a cached comparator value if
+ * it already exists (was not disposed with {@link
#disposeSortedIndexComparator(StorageSortedIndexDescriptor)}) for a given
descriptor.
+ */
+ public @Nullable JitComparator
createSortedIndexComparator(StorageSortedIndexDescriptor indexDescriptor) {
+ if (useLegacySortedIndexComparator) {
+ return null;
+ }
+
+ CachedComparator c =
cachedSortedIndexComparators.compute(indexDescriptor, (desc, cmp) -> {
+ if (cmp != null) {
+ return cmp.incrementUsage();
+ }
+
+ JitComparator jitComparator = createNewJitComparator(desc);
+
+ return new CachedComparator(jitComparator);
+ });
+
+ return c.jitComparator();
+ }
+
+ /**
+ * Marks that a comparator, created previously with {@link
#createSortedIndexComparator(StorageSortedIndexDescriptor)}, will no longer
+ * be used, and the internal cache of comparators may react to this
information by removing the comparator from the cache and freeing
+ * associated resources.
+ */
+ public void disposeSortedIndexComparator(StorageSortedIndexDescriptor
indexDescriptor) {
+ if (useLegacySortedIndexComparator) {
+ return;
+ }
+
+ cachedSortedIndexComparators.compute(indexDescriptor, (desc, cmp) -> {
+ assert cmp != null;
Review Comment:
Let's print some information about the problematic index here
##########
modules/storage-page-memory/src/main/java/org/apache/ignite/internal/storage/pagememory/AbstractPageMemoryStorageEngine.java:
##########
@@ -17,26 +17,173 @@
package org.apache.ignite.internal.storage.pagememory;
+import static java.util.Comparator.comparing;
+import static
org.apache.ignite.internal.util.IgniteUtils.lexicographicListComparator;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ConcurrentMap;
+import java.util.concurrent.ConcurrentSkipListMap;
import java.util.concurrent.atomic.AtomicLong;
+import org.apache.ignite.internal.catalog.descriptors.CatalogColumnCollation;
+import org.apache.ignite.internal.configuration.SystemLocalConfiguration;
+import org.apache.ignite.internal.configuration.SystemPropertyView;
import org.apache.ignite.internal.hlc.HybridClock;
import org.apache.ignite.internal.pagememory.PageMemory;
import org.apache.ignite.internal.pagememory.tree.BplusTree;
+import org.apache.ignite.internal.storage.StorageException;
import org.apache.ignite.internal.storage.engine.StorageEngine;
+import org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor;
+import
org.apache.ignite.internal.storage.index.StorageSortedIndexDescriptor.StorageSortedIndexColumnDescriptor;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorGenerator;
+import
org.apache.ignite.internal.storage.pagememory.index.sorted.comparator.JitComparatorOptions;
+import org.apache.ignite.internal.type.NativeType;
+import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.VisibleForTesting;
/** Abstract implementation of the storage engine based on memory {@link
PageMemory}. */
public abstract class AbstractPageMemoryStorageEngine implements StorageEngine
{
+ public static final String
LEGACY_PAGE_MEMERY_SORTED_INDEX_COMPARATOR_PROPERTY =
"legacyPageMemorySortedIndexComparator";
Review Comment:
Do we have a test that checks that everything is fine if we set this
property to "true"? I think no, because all inheritors override the `start`
method, but do not call `super.start()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
