From afed275f7df10118e161b8b5e496b012f67ec6d1 Mon Sep 17 00:00:00 2001 From: Daniel Mitterdorfer Date: Wed, 7 Feb 2024 08:38:26 +0100 Subject: [PATCH] [Profiling] Use plain arrays in stack traces With this commit we refactor the internal representation of stacktraces to use plain arrays instead of lists for some of its properties. The motivation behind this change is simplicity: * It avoids unnecessary boxing * We could eliminate a few redundant null checks because we use primitive types now in some places * We could slightly simplify runlength decoding --- .../profiling/GetStackTracesActionIT.java | 24 +++---- .../xpack/profiling/StackTrace.java | 65 +++++++++---------- .../TransportGetFlamegraphAction.java | 12 ++-- .../TransportGetStackTracesAction.java | 4 +- .../GetStackTracesResponseTests.java | 9 ++- .../xpack/profiling/StackTraceTests.java | 51 +++++++-------- .../TransportGetFlamegraphActionTests.java | 14 ++-- 7 files changed, 86 insertions(+), 93 deletions(-) diff --git a/x-pack/plugin/profiling/src/internalClusterTest/java/org/elasticsearch/xpack/profiling/GetStackTracesActionIT.java b/x-pack/plugin/profiling/src/internalClusterTest/java/org/elasticsearch/xpack/profiling/GetStackTracesActionIT.java index 6becc2eb6e385..a2274c952b4c3 100644 --- a/x-pack/plugin/profiling/src/internalClusterTest/java/org/elasticsearch/xpack/profiling/GetStackTracesActionIT.java +++ b/x-pack/plugin/profiling/src/internalClusterTest/java/org/elasticsearch/xpack/profiling/GetStackTracesActionIT.java @@ -29,10 +29,10 @@ public void testGetStackTracesUnfiltered() throws Exception { assertNotNull(response.getStackTraces()); // just do a high-level spot check. Decoding is tested in unit-tests StackTrace stackTrace = response.getStackTraces().get("L7kj7UvlKbT-vN73el4faQ"); - assertEquals(18, stackTrace.addressOrLines.size()); - assertEquals(18, stackTrace.fileIds.size()); - assertEquals(18, stackTrace.frameIds.size()); - assertEquals(18, stackTrace.typeIds.size()); + assertEquals(18, stackTrace.addressOrLines.length); + assertEquals(18, stackTrace.fileIds.length); + assertEquals(18, stackTrace.frameIds.length); + assertEquals(18, stackTrace.typeIds.length); assertEquals(0.0000048475146d, stackTrace.annualCO2Tons, 0.0000000001d); assertEquals(0.18834d, stackTrace.annualCostsUSD, 0.00001d); @@ -73,10 +73,10 @@ public void testGetStackTracesFromAPMWithMatchNoDownsampling() throws Exception assertNotNull(response.getStackTraces()); // just do a high-level spot check. Decoding is tested in unit-tests StackTrace stackTrace = response.getStackTraces().get("Ce77w10WeIDow3kd1jowlA"); - assertEquals(39, stackTrace.addressOrLines.size()); - assertEquals(39, stackTrace.fileIds.size()); - assertEquals(39, stackTrace.frameIds.size()); - assertEquals(39, stackTrace.typeIds.size()); + assertEquals(39, stackTrace.addressOrLines.length); + assertEquals(39, stackTrace.fileIds.length); + assertEquals(39, stackTrace.frameIds.length); + assertEquals(39, stackTrace.typeIds.length); assertTrue(stackTrace.annualCO2Tons > 0.0d); assertTrue(stackTrace.annualCostsUSD > 0.0d); @@ -139,10 +139,10 @@ public int hashCode() { assertNotNull(response.getStackTraces()); // just do a high-level spot check. Decoding is tested in unit-tests StackTrace stackTrace = response.getStackTraces().get("Ce77w10WeIDow3kd1jowlA"); - assertEquals(39, stackTrace.addressOrLines.size()); - assertEquals(39, stackTrace.fileIds.size()); - assertEquals(39, stackTrace.frameIds.size()); - assertEquals(39, stackTrace.typeIds.size()); + assertEquals(39, stackTrace.addressOrLines.length); + assertEquals(39, stackTrace.fileIds.length); + assertEquals(39, stackTrace.frameIds.length); + assertEquals(39, stackTrace.typeIds.length); assertTrue(stackTrace.annualCO2Tons > 0.0d); assertTrue(stackTrace.annualCostsUSD > 0.0d); diff --git a/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/StackTrace.java b/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/StackTrace.java index b417e267f12da..1cca3a4fe6417 100644 --- a/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/StackTrace.java +++ b/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/StackTrace.java @@ -12,9 +12,7 @@ import org.elasticsearch.xcontent.XContentBuilder; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.function.Consumer; @@ -25,20 +23,20 @@ final class StackTrace implements ToXContentObject { static final int NATIVE_FRAME_TYPE = 3; static final int KERNEL_FRAME_TYPE = 4; - List addressOrLines; - List fileIds; - List frameIds; - List typeIds; + int[] addressOrLines; + String[] fileIds; + String[] frameIds; + int[] typeIds; double annualCO2Tons; double annualCostsUSD; long count; StackTrace( - List addressOrLines, - List fileIds, - List frameIds, - List typeIds, + int[] addressOrLines, + String[] fileIds, + String[] frameIds, + int[] typeIds, double annualCO2Tons, double annualCostsUSD, long count @@ -84,8 +82,8 @@ final class StackTrace implements ToXContentObject { * @return Corresponding numbers that are encoded in the input. */ // package-private for testing - static List runLengthDecodeBase64Url(String input, int size, int capacity) { - Integer[] output = new Integer[capacity]; + static int[] runLengthDecodeBase64Url(String input, int size, int capacity) { + int[] output = new int[capacity]; int multipleOf8 = size / 8; int remainder = size % 8; @@ -138,7 +136,6 @@ static List runLengthDecodeBase64Url(String input, int size, int capaci value = n & 0xff; Arrays.fill(output, j, j + count, value); - j += count; } else if (remainder == 3) { n = (charCodeAt(input, i) << 12) | (charCodeAt(input, i + 1) << 6) | charCodeAt(input, i + 2); n >>= 2; @@ -147,12 +144,8 @@ static List runLengthDecodeBase64Url(String input, int size, int capaci value = n & 0xff; Arrays.fill(output, j, j + count, value); - j += count; - } - if (j < capacity) { - Arrays.fill(output, j, capacity, 0); } - return Arrays.asList(output); + return output; } // package-private for testing @@ -195,9 +188,9 @@ public static StackTrace fromSource(Map source) { String inputFrameTypes = ObjectPath.eval(PATH_FRAME_TYPES, source); int countsFrameIDs = inputFrameIDs.length() / BASE64_FRAME_ID_LENGTH; - List fileIDs = new ArrayList<>(countsFrameIDs); - List frameIDs = new ArrayList<>(countsFrameIDs); - List addressOrLines = new ArrayList<>(countsFrameIDs); + String[] fileIDs = new String[countsFrameIDs]; + String[] frameIDs = new String[countsFrameIDs]; + int[] addressOrLines = new int[countsFrameIDs]; // Step 1: Convert the base64-encoded frameID list into two separate // lists (frame IDs and file IDs), both of which are also base64-encoded. @@ -210,22 +203,22 @@ public static StackTrace fromSource(Map source) { // address (see diagram in definition of EncodedStackTrace). for (int i = 0, pos = 0; i < countsFrameIDs; i++, pos += BASE64_FRAME_ID_LENGTH) { String frameID = inputFrameIDs.substring(pos, pos + BASE64_FRAME_ID_LENGTH); - frameIDs.add(frameID); - fileIDs.add(getFileIDFromStackFrameID(frameID)); - addressOrLines.add(getAddressFromStackFrameID(frameID)); + frameIDs[i] = frameID; + fileIDs[i] = getFileIDFromStackFrameID(frameID); + addressOrLines[i] = getAddressFromStackFrameID(frameID); } // Step 2: Convert the run-length byte encoding into a list of uint8s. - List typeIDs = runLengthDecodeBase64Url(inputFrameTypes, inputFrameTypes.length(), countsFrameIDs); + int[] typeIDs = runLengthDecodeBase64Url(inputFrameTypes, inputFrameTypes.length(), countsFrameIDs); return new StackTrace(addressOrLines, fileIDs, frameIDs, typeIDs, 0, 0, 0); } public void forNativeAndKernelFrames(Consumer consumer) { - for (int i = 0; i < this.fileIds.size(); i++) { - Integer frameType = this.typeIds.get(i); - if (frameType != null && (frameType == NATIVE_FRAME_TYPE || frameType == KERNEL_FRAME_TYPE)) { - consumer.accept(this.fileIds.get(i)); + for (int i = 0; i < this.fileIds.length; i++) { + int frameType = this.typeIds[i]; + if (frameType == NATIVE_FRAME_TYPE || frameType == KERNEL_FRAME_TYPE) { + consumer.accept(this.fileIds[i]); } } } @@ -251,16 +244,20 @@ public boolean equals(Object o) { return false; } StackTrace that = (StackTrace) o; - return addressOrLines.equals(that.addressOrLines) - && fileIds.equals(that.fileIds) - && frameIds.equals(that.frameIds) - && typeIds.equals(that.typeIds); + return Arrays.equals(addressOrLines, that.addressOrLines) + && Arrays.equals(fileIds, that.fileIds) + && Arrays.equals(frameIds, that.frameIds) + && Arrays.equals(typeIds, that.typeIds); // Don't compare metadata like annualized co2, annualized costs and count. } // Don't hash metadata like annualized co2, annualized costs and count. @Override public int hashCode() { - return Objects.hash(addressOrLines, fileIds, frameIds, typeIds); + int result = Arrays.hashCode(addressOrLines); + result = 31 * result + Arrays.hashCode(fileIds); + result = 31 * result + Arrays.hashCode(frameIds); + result = 31 * result + Arrays.hashCode(typeIds); + return result; } } diff --git a/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphAction.java b/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphAction.java index dd78d6f1815f5..39b73db41aeef 100644 --- a/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphAction.java +++ b/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphAction.java @@ -91,12 +91,12 @@ static GetFlamegraphResponse buildFlamegraph(GetStackTracesResponse response) { builder.addAnnualCostsUSDInclusive(0, annualCostsUSD); builder.addAnnualCostsUSDExclusive(0, 0.0d); - int frameCount = stackTrace.frameIds.size(); + int frameCount = stackTrace.frameIds.length; for (int i = 0; i < frameCount; i++) { - String frameId = stackTrace.frameIds.get(i); - String fileId = stackTrace.fileIds.get(i); - Integer frameType = stackTrace.typeIds.get(i); - Integer addressOrLine = stackTrace.addressOrLines.get(i); + String frameId = stackTrace.frameIds[i]; + String fileId = stackTrace.fileIds[i]; + int frameType = stackTrace.typeIds[i]; + int addressOrLine = stackTrace.addressOrLines[i]; StackFrame stackFrame = response.getStackFrames().getOrDefault(frameId, EMPTY_STACKFRAME); String executable = response.getExecutables().getOrDefault(fileId, ""); final boolean isLeafFrame = i == frameCount - 1; @@ -199,7 +199,7 @@ public int addNode( int frameType, boolean inline, String fileName, - Integer addressOrLine, + int addressOrLine, String functionName, int functionOffset, String sourceFileName, diff --git a/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetStackTracesAction.java b/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetStackTracesAction.java index 2674893c2382f..cbb6f92fb417e 100644 --- a/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetStackTracesAction.java +++ b/x-pack/plugin/profiling/src/main/java/org/elasticsearch/xpack/profiling/TransportGetStackTracesAction.java @@ -567,8 +567,8 @@ public void onStackTraceResponse(MultiGetResponse multiGetItemResponses) { StackTrace stacktrace = StackTrace.fromSource(trace.getResponse().getSource()); // Guard against concurrent access and ensure we only handle each item once if (stackTracePerId.putIfAbsent(id, stacktrace) == null) { - totalFrames.addAndGet(stacktrace.frameIds.size()); - stackFrameIds.addAll(stacktrace.frameIds); + totalFrames.addAndGet(stacktrace.frameIds.length); + stackFrameIds.addAll(List.of(stacktrace.frameIds)); stacktrace.forNativeAndKernelFrames(e -> executableIds.add(e)); } } diff --git a/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/GetStackTracesResponseTests.java b/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/GetStackTracesResponseTests.java index 99a34719f96c9..3ebd2ef6a8aeb 100644 --- a/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/GetStackTracesResponseTests.java +++ b/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/GetStackTracesResponseTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.test.AbstractChunkedSerializingTestCase; import org.elasticsearch.test.ESTestCase; -import java.util.List; import java.util.Map; public class GetStackTracesResponseTests extends ESTestCase { @@ -25,10 +24,10 @@ private GetStackTracesResponse createTestInstance() { Map.of( "QjoLteG7HX3VUUXr-J4kHQ", new StackTrace( - List.of(1083999), - List.of("QCCDqjSg3bMK1C4YRK6Tiw"), - List.of("QCCDqjSg3bMK1C4YRK6TiwAAAAAAEIpf"), - List.of(2), + new int[] { 1083999 }, + new String[] { "QCCDqjSg3bMK1C4YRK6Tiw" }, + new String[] { "QCCDqjSg3bMK1C4YRK6TiwAAAAAAEIpf" }, + new int[] { 2 }, 0.3d, 2.7d, 1 diff --git a/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/StackTraceTests.java b/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/StackTraceTests.java index 4765d23bd30d0..4f583b55f18f7 100644 --- a/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/StackTraceTests.java +++ b/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/StackTraceTests.java @@ -16,8 +16,7 @@ import org.elasticsearch.xcontent.XContentType; import java.io.IOException; -import java.util.ArrayList; -import java.util.List; +import java.util.Arrays; import java.util.Map; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; @@ -33,30 +32,30 @@ public void testDecodeFrameId() { public void testRunlengthDecodeUniqueValues() { // 0 - 9 (reversed) String encodedFrameTypes = "AQkBCAEHAQYBBQEEAQMBAgEBAQA"; - List actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 10); - assertEquals(List.of(9, 8, 7, 6, 5, 4, 3, 2, 1, 0), actual); + int[] actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 10); + assertArrayEquals(new int[] { 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }, actual); } public void testRunlengthDecodeSingleValue() { // "4", repeated ten times String encodedFrameTypes = "CgQ"; - List actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 10); - assertEquals(List.of(4, 4, 4, 4, 4, 4, 4, 4, 4, 4), actual); + int[] actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 10); + assertArrayEquals(new int[] { 4, 4, 4, 4, 4, 4, 4, 4, 4, 4 }, actual); } public void testRunlengthDecodeFillsGap() { // "2", repeated three times String encodedFrameTypes = "AwI"; - List actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 5); + int[] actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 5); // zeroes should be appended for the last two values which are not present in the encoded representation. - assertEquals(List.of(2, 2, 2, 0, 0), actual); + assertArrayEquals(new int[] { 2, 2, 2, 0, 0 }, actual); } public void testRunlengthDecodeMixedValue() { // 4 String encodedFrameTypes = "BQADAg"; - List actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 8); - assertEquals(List.of(0, 0, 0, 0, 0, 2, 2, 2), actual); + int[] actual = StackTrace.runLengthDecodeBase64Url(encodedFrameTypes, encodedFrameTypes.length(), 8); + assertArrayEquals(new int[] { 0, 0, 0, 0, 0, 2, 2, 2 }, actual); } public void testCreateFromSource() { @@ -73,10 +72,10 @@ public void testCreateFromSource() { ) ); // end::noformat - assertEquals(List.of("AAAAAAAAAAUAAAAAAAAB3gAAAAAAD67u"), stackTrace.frameIds); - assertEquals(List.of("AAAAAAAAAAUAAAAAAAAB3g"), stackTrace.fileIds); - assertEquals(List.of(1027822), stackTrace.addressOrLines); - assertEquals(List.of(2), stackTrace.typeIds); + assertArrayEquals(new String[] { "AAAAAAAAAAUAAAAAAAAB3gAAAAAAD67u" }, stackTrace.frameIds); + assertArrayEquals(new String[] { "AAAAAAAAAAUAAAAAAAAB3g" }, stackTrace.fileIds); + assertArrayEquals(new int[] { 1027822 }, stackTrace.addressOrLines); + assertArrayEquals(new int[] { 2 }, stackTrace.typeIds); } public void testToXContent() throws IOException { @@ -94,10 +93,10 @@ public void testToXContent() throws IOException { XContentBuilder actualRequest = XContentFactory.contentBuilder(contentType); StackTrace stackTrace = new StackTrace( - List.of(1027822), - List.of("AAAAAAAAAAUAAAAAAAAB3g"), - List.of("AAAAAAAAAAUAAAAAAAAB3gAAAAAAD67u"), - List.of(2), + new int[] { 1027822 }, + new String[] { "AAAAAAAAAAUAAAAAAAAB3g" }, + new String[] { "AAAAAAAAAAUAAAAAAAAB3gAAAAAAD67u" }, + new int[] { 2 }, 0.3d, 2.7d, 1 @@ -109,10 +108,10 @@ public void testToXContent() throws IOException { public void testEquality() { StackTrace stackTrace = new StackTrace( - List.of(102782), - List.of("AAAAAAAAAAUAAAAAAAAB3g"), - List.of("AAAAAAAAAAUAAAAAAAAB3gAAAAAAD67u"), - List.of(2), + new int[] { 102782 }, + new String[] { "AAAAAAAAAAUAAAAAAAAB3g" }, + new String[] { "AAAAAAAAAAUAAAAAAAAB3gAAAAAAD67u" }, + new int[] { 2 }, 0.3d, 2.7d, 1 @@ -121,10 +120,10 @@ public void testEquality() { EqualsHashCodeTestUtils.checkEqualsAndHashCode( stackTrace, (o -> new StackTrace( - new ArrayList<>(o.addressOrLines), - new ArrayList<>(o.fileIds), - new ArrayList<>(o.frameIds), - new ArrayList<>(o.typeIds), + Arrays.copyOf(o.addressOrLines, o.addressOrLines.length), + Arrays.copyOf(o.fileIds, o.fileIds.length), + Arrays.copyOf(o.frameIds, o.frameIds.length), + Arrays.copyOf(o.typeIds, o.typeIds.length), 0.3d, 2.7d, 1 diff --git a/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphActionTests.java b/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphActionTests.java index 32735e5db935a..fd20ed04978f2 100644 --- a/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphActionTests.java +++ b/x-pack/plugin/profiling/src/test/java/org/elasticsearch/xpack/profiling/TransportGetFlamegraphActionTests.java @@ -18,8 +18,8 @@ public void testCreateFlamegraph() { Map.of( "2buqP1GpF-TXYmL4USW8gA", new StackTrace( - List.of(12784352, 19334053, 19336161, 18795859, 18622708, 18619213, 12989721, 13658842, 16339645), - List.of( + new int[] { 12784352, 19334053, 19336161, 18795859, 18622708, 18619213, 12989721, 13658842, 16339645 }, + new String[] { "fr28zxcZ2UDasxYuu6dV-w", "fr28zxcZ2UDasxYuu6dV-w", "fr28zxcZ2UDasxYuu6dV-w", @@ -28,9 +28,8 @@ public void testCreateFlamegraph() { "fr28zxcZ2UDasxYuu6dV-w", "fr28zxcZ2UDasxYuu6dV-w", "fr28zxcZ2UDasxYuu6dV-w", - "fr28zxcZ2UDasxYuu6dV-w" - ), - List.of( + "fr28zxcZ2UDasxYuu6dV-w" }, + new String[] { "fr28zxcZ2UDasxYuu6dV-wAAAAAAwxLg", "fr28zxcZ2UDasxYuu6dV-wAAAAABJwOl", "fr28zxcZ2UDasxYuu6dV-wAAAAABJwvh", @@ -39,9 +38,8 @@ public void testCreateFlamegraph() { "fr28zxcZ2UDasxYuu6dV-wAAAAABHBtN", "fr28zxcZ2UDasxYuu6dV-wAAAAAAxjUZ", "fr28zxcZ2UDasxYuu6dV-wAAAAAA0Gra", - "fr28zxcZ2UDasxYuu6dV-wAAAAAA-VK9" - ), - List.of(3, 3, 3, 3, 3, 3, 3, 3, 3), + "fr28zxcZ2UDasxYuu6dV-wAAAAAA-VK9" }, + new int[] { 3, 3, 3, 3, 3, 3, 3, 3, 3 }, 0.3d, 2.7d, 1