iprithv commented on code in PR #16201:
URL: https://github.com/apache/lucene/pull/16201#discussion_r3368350327
##########
lucene/core/src/java/org/apache/lucene/internal/hppc/IntObjectHashMap.java:
##########
@@ -562,7 +562,7 @@ public ValuesContainer values() {
}
/** A view over the set of values of this map. */
- public final class ValuesContainer implements Iterable<ObjectCursor<VType>> {
+ public class ValuesContainer implements Iterable<ObjectCursor<VType>> {
Review Comment:
not sure if this is good. we should use final and use composition in
ReadOnlyDenseIntObjectMap..
##########
lucene/core/src/java/org/apache/lucene/internal/hppc/ReadOnlyDenseIntObjectMap.java:
##########
@@ -0,0 +1,295 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.internal.hppc;
+
+import java.util.Arrays;
+import java.util.Iterator;
+import org.apache.lucene.util.RamUsageEstimator;
+
+/**
+ * A read-only dense map of non-negative int keys to non-null object values.
+ *
+ * <p>This map stores values directly in an {@code Object[]} indexed by key.
It is intended for
+ * field-number maps that are built once with {@link IntObjectHashMap} and
then only queried.
+ *
+ * @lucene.internal
+ */
+@SuppressWarnings("unchecked")
+public final class ReadOnlyDenseIntObjectMap<VType> extends
IntObjectHashMap<VType> {
+
+ // Avoid changing the map implementation unless the dense array removes
meaningful slack.
+ private static final int MIN_SLOT_SAVINGS_PERCENT = 30;
+
+ private static final long BASE_RAM_BYTES_USED =
+ RamUsageEstimator.shallowSizeOfInstance(ReadOnlyDenseIntObjectMap.class);
+
+ public static <VType> IntObjectHashMap<VType>
maybeWrap(IntObjectHashMap<VType> map) {
+ return maybeWrap(map, MIN_SLOT_SAVINGS_PERCENT);
+ }
+
+ public static <VType> IntObjectHashMap<VType> maybeWrap(
+ IntObjectHashMap<VType> map, int minSlotSavingsPercent) {
+ if (minSlotSavingsPercent < 0 || minSlotSavingsPercent > 100) {
+ throw new IllegalArgumentException(
+ "minSlotSavingsPercent must be in [0, 100]: " +
minSlotSavingsPercent);
+ }
+ int maxKey = -1;
+ for (IntObjectCursor<VType> cursor : map) {
+ if (cursor.key < 0 || cursor.value == null) {
+ return map;
+ }
+ maxKey = Math.max(maxKey, cursor.key);
+ }
+ if (maxKey == Integer.MAX_VALUE) {
+ return map;
+ }
+ int denseSlots = maxKey + 1;
+ int savedSlots = map.values.length - denseSlots;
+ return savedSlots * 100L >= (long) map.values.length *
minSlotSavingsPercent
+ ? new ReadOnlyDenseIntObjectMap<>(map)
+ : map;
+ }
+
+ public ReadOnlyDenseIntObjectMap(IntObjectHashMap<? extends VType> map) {
+ super(0);
+
+ int maxKey = -1;
+ for (IntObjectCursor<? extends VType> cursor : map) {
+ if (cursor.key < 0) {
+ throw new IllegalArgumentException("Dense int map only supports
non-negative keys");
+ }
+ if (cursor.value == null) {
+ throw new IllegalArgumentException("Dense int map does not support
null values");
+ }
+ maxKey = Math.max(maxKey, cursor.key);
+ }
+ keys = null;
+ values = new Object[maxKey + 1];
+ for (IntObjectCursor<? extends VType> cursor : map) {
+ values[cursor.key] = cursor.value;
+ }
+ assigned = map.size();
+ hasEmptyKey = values.length > 0 && values[0] != null;
+ }
+
+ @Override
+ public VType put(int key, VType value) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public boolean putIfAbsent(int key, VType value) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public VType remove(int key) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public int putAll(Iterable<? extends IntObjectCursor<? extends VType>>
iterable) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public VType indexReplace(int index, VType newValue) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public void indexInsert(int index, int key, VType value) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public VType indexRemove(int index) {
+ throw new UnsupportedOperationException("This map is read-only");
+ }
+
+ @Override
+ public VType get(int key) {
+ return key >= 0 && key < values.length ? (VType) values[key] : null;
+ }
+
+ @Override
+ public VType getOrDefault(int key, VType defaultValue) {
+ VType value = get(key);
+ return value == null ? defaultValue : value;
+ }
+
+ @Override
+ public boolean containsKey(int key) {
+ return get(key) != null;
+ }
+
+ @Override
+ public int indexOf(int key) {
+ if (containsKey(key)) {
+ return key;
+ }
+ return key >= 0 ? ~key : -1;
+ }
+
+ @Override
+ public boolean indexExists(int index) {
+ return index >= 0 && index < values.length && values[index] != null;
+ }
+
+ @Override
+ public VType indexGet(int index) {
+ assert indexExists(index) : "The index must point at an existing key.";
+ return (VType) values[index];
+ }
+
+ @Override
+ public void clear() {
+ Arrays.fill(values, null);
+ assigned = 0;
+ hasEmptyKey = false;
+ }
+
+ @Override
+ public void release() {
+ values = new Object[0];
+ assigned = 0;
+ hasEmptyKey = false;
+ }
+
+ @Override
+ public void ensureCapacity(int expectedElements) {
+ if (keys == null && assigned == 0 && resizeAt == 0) {
+ super.ensureCapacity(expectedElements);
Review Comment:
this couples to the internal initialization order of IntObjectHashMap fields
and I guess will silently break if any superclass refactoring changes this
order or adds an early ensureCapacity call. if this condition fails,
constructor allocates buffers that are immediately thrown away by the
subsequent constructor body, wasting memory.
should we not extend IntObjectHashMap at all? Instead, use composition (hold
the dense Object[] in a standalone class) or a static factory that returns a
common interface/sealed type. extending a mutable hash map to make a read-only
dense array is not ideal... callers holding an IntObjectHashMap reference will
not expect put() to throw UnsupportedOperationException. this subclassing
approach also forces to remove final from KeysContainer and ValuesContainer in
the parent class..
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
