From 249031c372dd6e387e9482317485a2858df7bdb2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20B=C3=BCscher?= Date: Mon, 30 Oct 2023 16:31:03 +0100 Subject: [PATCH] Revert "Optimize ContentPath#pathAsText (#98244)" (#101521) Some benchmarks indicate that parts of the change made in #98244 is negatively affecting document parsing. This change reverts it until we have better benchmarks that support it or a solution that avoids the Stack datastructure most likely contributing to this. This reverts commit db22afa. --- docs/changelog/98244.yaml | 6 --- .../index/mapper/ContentPath.java | 48 +++++++++-------- .../index/mapper/ContentPathTests.java | 51 ++----------------- 3 files changed, 31 insertions(+), 74 deletions(-) delete mode 100644 docs/changelog/98244.yaml diff --git a/docs/changelog/98244.yaml b/docs/changelog/98244.yaml deleted file mode 100644 index e1dde59a83e4..000000000000 --- a/docs/changelog/98244.yaml +++ /dev/null @@ -1,6 +0,0 @@ -pr: 98244 -summary: Optimize ContentPath#pathAsText -area: Search -type: enhancement -issues: - - 94544 diff --git a/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java b/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java index 1386028b4ea5..e90e2bbe1afd 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/ContentPath.java @@ -8,35 +8,42 @@ package org.elasticsearch.index.mapper; -import java.util.Stack; - public final class ContentPath { private static final char DELIMITER = '.'; private final StringBuilder sb; - private final Stack delimiterIndexes; + + private int index = 0; + + private String[] path = new String[10]; + private boolean withinLeafObject = false; public ContentPath() { this.sb = new StringBuilder(); - this.delimiterIndexes = new Stack<>(); } - public void add(String name) { - // Store the location of the previous final delimiter onto the stack, - // which will be the index of the 2nd last delimiter after appending the new name - delimiterIndexes.add(sb.length() - 1); - sb.append(name).append(DELIMITER); + String[] getPath() { + // used for testing + return path; } - public void remove() { - if (delimiterIndexes.isEmpty()) { - throw new IllegalStateException("Content path is empty"); + public void add(String name) { + path[index++] = name; + if (index == path.length) { // expand if needed + expand(); } + } + + private void expand() { + String[] newPath = new String[path.length + 10]; + System.arraycopy(path, 0, newPath, 0, path.length); + path = newPath; + } - // Deletes the last node added to the stringbuilder by deleting from the 2nd last delimiter onwards - sb.setLength(delimiterIndexes.pop() + 1); + public void remove() { + path[--index] = null; } public void setWithinLeafObject(boolean withinLeafObject) { @@ -48,16 +55,15 @@ public boolean isWithinLeafObject() { } public String pathAsText(String name) { - // If length is 0 we know that we are at the root, so return the provided string directly - if (length() == 0) { - return name; + sb.setLength(0); + for (int i = 0; i < index; i++) { + sb.append(path[i]).append(DELIMITER); } - - return sb + name; + sb.append(name); + return sb.toString(); } public int length() { - // The amount of delimiters we've added tells us the amount of nodes that have been added to the path - return delimiterIndexes.size(); + return index; } } diff --git a/server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java b/server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java index d66d927e8e0d..829e2fcbe79d 100644 --- a/server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java +++ b/server/src/test/java/org/elasticsearch/index/mapper/ContentPathTests.java @@ -23,7 +23,10 @@ public void testAddPath() { public void testRemovePath() { ContentPath contentPath = new ContentPath(); contentPath.add("foo"); + String[] path = contentPath.getPath(); + assertEquals("foo", path[0]); contentPath.remove(); + assertNull(path[0]); assertEquals(0, contentPath.length()); String pathAsText = contentPath.pathAsText("bar"); assertEquals("bar", pathAsText); @@ -33,21 +36,7 @@ public void testRemovePathException() { ContentPath contentPath = new ContentPath(); contentPath.add("foo"); contentPath.remove(); - expectThrows(IllegalStateException.class, contentPath::remove); - } - - public void testRootPath() { - ContentPath contentPath = new ContentPath(); - assertEquals("root", contentPath.pathAsText("root")); - assertEquals(0, contentPath.length()); - } - - public void testNestedPath() { - ContentPath contentPath = new ContentPath(); - contentPath.add("root"); - contentPath.add("inner"); - assertEquals("root.inner.leaf1", contentPath.pathAsText("leaf1")); - assertEquals("root.inner.leaf2", contentPath.pathAsText("leaf2")); + expectThrows(IndexOutOfBoundsException.class, contentPath::remove); } public void testBehaviourWithLongerPath() { @@ -93,15 +82,6 @@ public void testPathAsText() { assertEquals("foo.bar.baz", contentPath.pathAsText("baz")); } - public void testPathTextAfterLeafRemoval() { - ContentPath contentPath = new ContentPath(); - contentPath.add("root"); - contentPath.add("inner"); - contentPath.add("leaf"); - contentPath.remove(); - assertEquals("root.inner.newLeaf", contentPath.pathAsText("newLeaf")); - } - public void testPathAsTextAfterRemove() { ContentPath contentPath = new ContentPath(); contentPath.add("foo"); @@ -120,27 +100,4 @@ public void testPathAsTextAfterRemoveAndMoreAdd() { contentPath.add("baz"); assertEquals("foo.baz.qux", contentPath.pathAsText("qux")); } - - public void testPathTextAfterRootRemovalAndNewPathAdded() { - ContentPath contentPath = new ContentPath(); - contentPath.add("root"); - contentPath.add("inner"); - contentPath.add("leaf"); - contentPath.remove(); - contentPath.remove(); - contentPath.remove(); - contentPath.add("newRoot"); - contentPath.add("newInner"); - assertEquals("newRoot.newInner.newLeaf", contentPath.pathAsText("newLeaf")); - } - - public void testPathTextRemovalAfterPathAsTextHasBeenCalled() { - ContentPath contentPath = new ContentPath(); - contentPath.add("root"); - contentPath.add("inner"); - contentPath.pathAsText("leaf"); - contentPath.remove(); - contentPath.add("newInner"); - assertEquals("root.newInner.newLeaf", contentPath.pathAsText("newLeaf")); - } }