From 457d75ccba373e88f9913c17c62b1680bddcec5d Mon Sep 17 00:00:00 2001 From: Nick Knize Date: Tue, 11 Apr 2023 13:23:48 -0500 Subject: [PATCH] [Refactor] SegmentsStats#filesSizes from ImmutableOpenMap to java.util.Map (#7094) Using an ImmutableOpenMap in filesSizes in o.o.index.engine.SegmentsStats is overkill since all we care about is public facing API not leaking the filesSizes collection. This commit refactors the SegmentsStats#filesSizes variable from an ImmutableOpenMap to an unmodifiable java.util.Map to remove all of the unnecessary temporary object copies on the internal map. It also refactors the internal only used FILE_DESCRIPTIONS static variable to an unmodifiable java.util.Map in order to completely remove SegmentsStats dependency on hppc based ImmutableOpenMap. Signed-off-by: Nicholas Walter Knize --- .../org/opensearch/index/engine/Engine.java | 14 +-- .../index/engine/SegmentsStats.java | 106 ++++++++---------- .../index/engine/InternalEngineTests.java | 7 +- 3 files changed, 55 insertions(+), 72 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/engine/Engine.java b/server/src/main/java/org/opensearch/index/engine/Engine.java index a0bd5e1ca6dad..80f7c353d4a60 100644 --- a/server/src/main/java/org/opensearch/index/engine/Engine.java +++ b/server/src/main/java/org/opensearch/index/engine/Engine.java @@ -60,7 +60,6 @@ import org.opensearch.common.Nullable; import org.opensearch.common.SetOnce; import org.opensearch.common.bytes.BytesReference; -import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.concurrent.GatedCloseable; import org.opensearch.common.lease.Releasable; import org.opensearch.common.lease.Releasables; @@ -99,6 +98,7 @@ import java.io.UncheckedIOException; import java.nio.file.NoSuchFileException; import java.util.Arrays; +import java.util.Collections; import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; @@ -950,7 +950,7 @@ protected void fillSegmentStats(SegmentReader segmentReader, boolean includeSegm } } - private ImmutableOpenMap getSegmentFileSizes(SegmentReader segmentReader) { + private Map getSegmentFileSizes(SegmentReader segmentReader) { Directory directory = null; SegmentCommitInfo segmentCommitInfo = segmentReader.getSegmentInfo(); boolean useCompoundFile = segmentCommitInfo.info.getUseCompoundFile(); @@ -969,7 +969,7 @@ private ImmutableOpenMap getSegmentFileSizes(SegmentReader segment e ); - return ImmutableOpenMap.of(); + return Map.of(); } } else { directory = segmentReader.directory(); @@ -984,7 +984,7 @@ private ImmutableOpenMap getSegmentFileSizes(SegmentReader segment } catch (IOException e) { final Directory finalDirectory = directory; logger.warn(() -> new ParameterizedMessage("Couldn't list Compound Reader Directory [{}]", finalDirectory), e); - return ImmutableOpenMap.of(); + return Map.of(); } } else { try { @@ -998,11 +998,11 @@ private ImmutableOpenMap getSegmentFileSizes(SegmentReader segment ), e ); - return ImmutableOpenMap.of(); + return Map.of(); } } - ImmutableOpenMap.Builder map = ImmutableOpenMap.builder(); + Map map = new HashMap<>(); for (String file : files) { String extension = IndexFileNames.getExtension(file); long length = 0L; @@ -1033,7 +1033,7 @@ private ImmutableOpenMap getSegmentFileSizes(SegmentReader segment } } - return map.build(); + return Collections.unmodifiableMap(map); } protected void writerSegmentStats(SegmentsStats stats) { diff --git a/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java b/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java index ba0bc32ddbf4f..aeb67f1ee41a2 100644 --- a/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java +++ b/server/src/main/java/org/opensearch/index/engine/SegmentsStats.java @@ -32,9 +32,7 @@ package org.opensearch.index.engine; -import com.carrotsearch.hppc.cursors.ObjectObjectCursor; import org.opensearch.Version; -import org.opensearch.common.collect.ImmutableOpenMap; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; import org.opensearch.common.io.stream.Writeable; @@ -43,6 +41,9 @@ import org.opensearch.core.xcontent.XContentBuilder; import java.io.IOException; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; /** * Tracker for segment stats @@ -56,7 +57,7 @@ public class SegmentsStats implements Writeable, ToXContentFragment { private long versionMapMemoryInBytes; private long maxUnsafeAutoIdTimestamp = Long.MIN_VALUE; private long bitsetMemoryInBytes; - private ImmutableOpenMap fileSizes = ImmutableOpenMap.of(); + private final Map fileSizes; private static final ByteSizeValue ZERO_BYTE_SIZE_VALUE = new ByteSizeValue(0L); @@ -66,29 +67,31 @@ public class SegmentsStats implements Writeable, ToXContentFragment { * Ideally this should be in sync to what the current version of Lucene is using, but it's harmless to leave extensions out, * they'll just miss a proper description in the stats */ - private static final ImmutableOpenMap FILE_DESCRIPTIONS = ImmutableOpenMap.builder() - .fPut("si", "Segment Info") - .fPut("fnm", "Fields") - .fPut("fdx", "Field Index") - .fPut("fdt", "Field Data") - .fPut("tim", "Term Dictionary") - .fPut("tip", "Term Index") - .fPut("doc", "Frequencies") - .fPut("pos", "Positions") - .fPut("pay", "Payloads") - .fPut("nvd", "Norms") - .fPut("nvm", "Norms") - .fPut("dii", "Points") - .fPut("dim", "Points") - .fPut("dvd", "DocValues") - .fPut("dvm", "DocValues") - .fPut("tvx", "Term Vector Index") - .fPut("tvd", "Term Vector Documents") - .fPut("tvf", "Term Vector Fields") - .fPut("liv", "Live Documents") - .build(); - - public SegmentsStats() {} + private static final Map FILE_DESCRIPTIONS = Map.ofEntries( + Map.entry("si", "Segment Info"), + Map.entry("fnm", "Fields"), + Map.entry("fdx", "Field Index"), + Map.entry("fdt", "Field Data"), + Map.entry("tim", "Term Dictionary"), + Map.entry("tip", "Term Index"), + Map.entry("doc", "Frequencies"), + Map.entry("pos", "Positions"), + Map.entry("pay", "Payloads"), + Map.entry("nvd", "Norms"), + Map.entry("nvm", "Norms"), + Map.entry("dii", "Points"), + Map.entry("dim", "Points"), + Map.entry("dvd", "DocValues"), + Map.entry("dvm", "DocValues"), + Map.entry("tvx", "Term Vector Index"), + Map.entry("tvd", "Term Vector Documents"), + Map.entry("tvf", "Term Vector Fields"), + Map.entry("liv", "Live Documents") + ); + + public SegmentsStats() { + fileSizes = new HashMap<>(); + } public SegmentsStats(StreamInput in) throws IOException { count = in.readVLong(); @@ -107,15 +110,7 @@ public SegmentsStats(StreamInput in) throws IOException { versionMapMemoryInBytes = in.readLong(); bitsetMemoryInBytes = in.readLong(); maxUnsafeAutoIdTimestamp = in.readLong(); - - int size = in.readVInt(); - ImmutableOpenMap.Builder map = ImmutableOpenMap.builder(size); - for (int i = 0; i < size; i++) { - String key = in.readString(); - Long value = in.readLong(); - map.put(key, value); - } - fileSizes = map.build(); + fileSizes = in.readMap(StreamInput::readString, StreamInput::readLong); } public void add(long count) { @@ -138,19 +133,12 @@ public void addBitsetMemoryInBytes(long bitsetMemoryInBytes) { this.bitsetMemoryInBytes += bitsetMemoryInBytes; } - public void addFileSizes(ImmutableOpenMap fileSizes) { - ImmutableOpenMap.Builder map = ImmutableOpenMap.builder(this.fileSizes); - - for (ObjectObjectCursor entry : fileSizes) { - if (map.containsKey(entry.key)) { - Long oldValue = map.get(entry.key); - map.put(entry.key, oldValue + entry.value); - } else { - map.put(entry.key, entry.value); - } - } - - this.fileSizes = map.build(); + public void addFileSizes(final Map newFileSizes) { + newFileSizes.forEach((k, v) -> this.fileSizes.merge(k, v, (a, b) -> { + assert a != null; + assert b != null; + return Math.addExact(a, b); + })); } public void add(SegmentsStats mergeStats) { @@ -205,8 +193,9 @@ public ByteSizeValue getBitsetMemory() { return new ByteSizeValue(bitsetMemoryInBytes); } - public ImmutableOpenMap getFileSizes() { - return fileSizes; + /** Returns mapping of file names to their size (only used in tests) */ + public Map getFileSizes() { + return Collections.unmodifiableMap(this.fileSizes); } /** @@ -233,10 +222,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.humanReadableField(Fields.FIXED_BIT_SET_MEMORY_IN_BYTES, Fields.FIXED_BIT_SET, getBitsetMemory()); builder.field(Fields.MAX_UNSAFE_AUTO_ID_TIMESTAMP, maxUnsafeAutoIdTimestamp); builder.startObject(Fields.FILE_SIZES); - for (ObjectObjectCursor entry : fileSizes) { - builder.startObject(entry.key); - builder.humanReadableField(Fields.SIZE_IN_BYTES, Fields.SIZE, new ByteSizeValue(entry.value)); - builder.field(Fields.DESCRIPTION, FILE_DESCRIPTIONS.getOrDefault(entry.key, "Others")); + for (Map.Entry entry : fileSizes.entrySet()) { + builder.startObject(entry.getKey()); + builder.humanReadableField(Fields.SIZE_IN_BYTES, Fields.SIZE, new ByteSizeValue(entry.getValue())); + builder.field(Fields.DESCRIPTION, FILE_DESCRIPTIONS.getOrDefault(entry.getKey(), "Others")); builder.endObject(); } builder.endObject(); @@ -297,16 +286,11 @@ public void writeTo(StreamOutput out) throws IOException { out.writeLong(versionMapMemoryInBytes); out.writeLong(bitsetMemoryInBytes); out.writeLong(maxUnsafeAutoIdTimestamp); - - out.writeVInt(fileSizes.size()); - for (ObjectObjectCursor entry : fileSizes) { - out.writeString(entry.key); - out.writeLong(entry.value); - } + out.writeMap(this.fileSizes, StreamOutput::writeString, StreamOutput::writeLong); } public void clearFileSizes() { - fileSizes = ImmutableOpenMap.of(); + fileSizes.clear(); } /** diff --git a/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java index 4b0f55f47d4cf..a1e1937f78c32 100644 --- a/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java @@ -32,7 +32,6 @@ package org.opensearch.index.engine; -import com.carrotsearch.hppc.cursors.ObjectObjectCursor; import com.carrotsearch.randomizedtesting.generators.RandomNumbers; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; @@ -430,15 +429,15 @@ public void testSegmentsStatsIncludingFileSizes() throws Exception { SegmentsStats stats = engine.segmentsStats(true, false); assertThat(stats.getFileSizes().size(), greaterThan(0)); - assertThat(() -> stats.getFileSizes().valuesIt(), everyItem(greaterThan(0L))); + assertThat(() -> stats.getFileSizes().values().iterator(), everyItem(greaterThan(0L))); - ObjectObjectCursor firstEntry = stats.getFileSizes().iterator().next(); + Map.Entry firstEntry = stats.getFileSizes().entrySet().iterator().next(); ParsedDocument doc2 = testParsedDocument("2", null, testDocumentWithTextField(), B_2, null); engine.index(indexForDoc(doc2)); engine.refresh("test"); - assertThat(engine.segmentsStats(true, false).getFileSizes().get(firstEntry.key), greaterThan(firstEntry.value)); + assertThat(engine.segmentsStats(true, false).getFileSizes().get(firstEntry.getKey()), greaterThan(firstEntry.getValue())); } }