Skip to content

Commit

Permalink
[Refactor] SegmentsStats#filesSizes from ImmutableOpenMap to java.uti…
Browse files Browse the repository at this point in the history 
…l.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 <nknize@apache.org>
  • Loading branch information
@nknize
nknize authored Apr 11, 2023
1 parent 64f3123 commit 352de4e
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 72 deletions.
14 changes: 7 additions & 7 deletions server/src/main/java/org/opensearch/index/engine/Engine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -950,7 +950,7 @@ protected void fillSegmentStats(SegmentReader segmentReader, boolean includeSegm
}
}

private ImmutableOpenMap<String, Long> getSegmentFileSizes(SegmentReader segmentReader) {
private Map<String, Long> getSegmentFileSizes(SegmentReader segmentReader) {
Directory directory = null;
SegmentCommitInfo segmentCommitInfo = segmentReader.getSegmentInfo();
boolean useCompoundFile = segmentCommitInfo.info.getUseCompoundFile();
Expand All @@ -969,7 +969,7 @@ private ImmutableOpenMap<String, Long> getSegmentFileSizes(SegmentReader segment
e
);

return ImmutableOpenMap.of();
return Map.of();
}
} else {
directory = segmentReader.directory();
Expand All @@ -984,7 +984,7 @@ private ImmutableOpenMap<String, Long> 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 {
Expand All @@ -998,11 +998,11 @@ private ImmutableOpenMap<String, Long> getSegmentFileSizes(SegmentReader segment
),
e
);
return ImmutableOpenMap.of();
return Map.of();
}
}

ImmutableOpenMap.Builder<String, Long> map = ImmutableOpenMap.builder();
Map<String, Long> map = new HashMap<>();
for (String file : files) {
String extension = IndexFileNames.getExtension(file);
long length = 0L;
Expand Down Expand Up @@ -1033,7 +1033,7 @@ private ImmutableOpenMap<String, Long> getSegmentFileSizes(SegmentReader segment
}
}

return map.build();
return Collections.unmodifiableMap(map);
}

protected void writerSegmentStats(SegmentsStats stats) {
Expand Down
106 changes: 45 additions & 61 deletions server/src/main/java/org/opensearch/index/engine/SegmentsStats.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -56,7 +57,7 @@ public class SegmentsStats implements Writeable, ToXContentFragment {
private long versionMapMemoryInBytes;
private long maxUnsafeAutoIdTimestamp = Long.MIN_VALUE;
private long bitsetMemoryInBytes;
private ImmutableOpenMap<String, Long> fileSizes = ImmutableOpenMap.of();
private final Map<String, Long> fileSizes;

private static final ByteSizeValue ZERO_BYTE_SIZE_VALUE = new ByteSizeValue(0L);

Expand All @@ -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<String, String> FILE_DESCRIPTIONS = ImmutableOpenMap.<String, String>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<String, String> 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();
Expand All @@ -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<String, Long> 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) {
Expand All @@ -138,19 +133,12 @@ public void addBitsetMemoryInBytes(long bitsetMemoryInBytes) {
this.bitsetMemoryInBytes += bitsetMemoryInBytes;
}

public void addFileSizes(ImmutableOpenMap<String, Long> fileSizes) {
ImmutableOpenMap.Builder<String, Long> map = ImmutableOpenMap.builder(this.fileSizes);

for (ObjectObjectCursor<String, Long> 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<String, Long> 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) {
Expand Down Expand Up @@ -205,8 +193,9 @@ public ByteSizeValue getBitsetMemory() {
return new ByteSizeValue(bitsetMemoryInBytes);
}

public ImmutableOpenMap<String, Long> getFileSizes() {
return fileSizes;
/** Returns mapping of file names to their size (only used in tests) */
public Map<String, Long> getFileSizes() {
return Collections.unmodifiableMap(this.fileSizes);
}

/**
Expand All @@ -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<String, Long> 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<String, Long> 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();
Expand Down Expand Up @@ -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<String, Long> 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();
}

/**
Expand Down
7 changes: 3 additions & 4 deletions server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String, Long> firstEntry = stats.getFileSizes().iterator().next();
Map.Entry<String, Long> 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()));
}
}

Expand Down

0 comments on commit 352de4e

Please sign in to comment.