From b067247fad5040b8e451e804553bdbb5f7e50f38 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 11:52:59 +0100 Subject: [PATCH 01/11] [test] Add test for over-allocation of components size --- .../src/main/java/org/exist/storage/NodePath.java | 14 +++++++++++++- .../test/java/org/exist/storage/NodePathTest.java | 6 ++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index 1565c18fcdc..61e8577052d 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -38,7 +38,7 @@ */ public class NodePath implements Comparable { - private static final int DEFAULT_NODE_PATH_SIZE = 5; + static final int DEFAULT_NODE_PATH_SIZE = 5; private static final Logger LOG = LogManager.getLogger(NodePath.class); @@ -130,6 +130,18 @@ public int length() { return pos; } + /** + * Return the size of the components array. + * + * This function is intentionally marked as package-private + * so that it may only be called for testing purposes! + * + * @return the size of the components array. + */ + int componentsSize() { + return components.length; + } + protected void reverseComponents() { for (int i = 0; i < pos / 2; ++i) { QName tmp = components[i]; diff --git a/exist-core/src/test/java/org/exist/storage/NodePathTest.java b/exist-core/src/test/java/org/exist/storage/NodePathTest.java index 11ef3aa9c7a..8da175c1c78 100644 --- a/exist-core/src/test/java/org/exist/storage/NodePathTest.java +++ b/exist-core/src/test/java/org/exist/storage/NodePathTest.java @@ -109,5 +109,11 @@ public void appendFromNonEmpty() { assertEquals("/a/b/c/d/1/2/3", path.toString()); } + @Test + public void largerComponentsBuffer() { + final NodePath path = new NodePath(null, "/a"); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); + } + } From 9932d435cb11db61e976d794ec403fdb98a2fb68 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 11:53:42 +0100 Subject: [PATCH 02/11] [test] Add test for reset strategy for over-allocation --- .../main/java/org/exist/storage/NodePath.java | 5 ++-- .../java/org/exist/storage/NodePathTest.java | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index 61e8577052d..48bdd7a6733 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -39,6 +39,7 @@ public class NodePath implements Comparable { static final int DEFAULT_NODE_PATH_SIZE = 5; + static final int MAX_OVER_ALLOCATION_FACTOR = 2; private static final Logger LOG = LogManager.getLogger(NodePath.class); @@ -117,8 +118,8 @@ public void removeLastComponent() { } public void reset() { - // when resetting if this object has twice the capacity of a new object, then set it back to the default capacity - if (pos > DEFAULT_NODE_PATH_SIZE * 2) { + // when resetting if this object has more than twice the capacity of a new object, then set it back to the default capacity + if (pos > DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR) { components = new QName[DEFAULT_NODE_PATH_SIZE]; } else { Arrays.fill(components, null); diff --git a/exist-core/src/test/java/org/exist/storage/NodePathTest.java b/exist-core/src/test/java/org/exist/storage/NodePathTest.java index 8da175c1c78..f1609d846ea 100644 --- a/exist-core/src/test/java/org/exist/storage/NodePathTest.java +++ b/exist-core/src/test/java/org/exist/storage/NodePathTest.java @@ -115,5 +115,35 @@ public void largerComponentsBuffer() { assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); } + @Test + public void reset() { + // simple allocation of DEFAULT_NODE_PATH_SIZE and then reset: size of components should remain at DEFAULT_NODE_PATH_SIZE + NodePath path = new NodePath(null, "/a"); + path.reset(); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); + + // another simple allocation of DEFAULT_NODE_PATH_SIZE and then reset: size of components should remain at DEFAULT_NODE_PATH_SIZE + path = new NodePath(null, "/a/b"); + path.reset(); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); + + // allocate upto DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR and then reset: size of components should remain at DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR + StringBuilder pathStrBuilder = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR; i++) { + pathStrBuilder.append("/a"); + } + path = new NodePath(null, pathStrBuilder.toString()); + path.reset(); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR, path.componentsSize()); + + // allocate over DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR and then reset: size of components should return to DEFAULT_NODE_PATH_SIZE + pathStrBuilder = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR * 2; i++) { + pathStrBuilder.append("/a"); + } + path = new NodePath(null, pathStrBuilder.toString()); + path.reset(); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); + } } From 1babfc4e18070c1fd4f14322c381e177fa8c505c Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 13:10:40 +0100 Subject: [PATCH 03/11] [test] Test for bug in NodePath where NodePath equality fails after over-allocation --- .../java/org/exist/storage/NodePathTest.java | 107 ++++++++++++++++++ 1 file changed, 107 insertions(+) diff --git a/exist-core/src/test/java/org/exist/storage/NodePathTest.java b/exist-core/src/test/java/org/exist/storage/NodePathTest.java index f1609d846ea..cd5e00b3714 100644 --- a/exist-core/src/test/java/org/exist/storage/NodePathTest.java +++ b/exist-core/src/test/java/org/exist/storage/NodePathTest.java @@ -146,4 +146,111 @@ public void reset() { assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); } + @Test + public void equalsAfterResetAndAppend() { + final String strPath = "/a/b/c"; + + final NodePath expected = new NodePath(null, strPath); + + // simple path, reset, and then append: expected.equals(actual) should equal 'true' + NodePath actual = new NodePath(null, "/a"); + actual.reset(); + actual.append(expected); + assertEquals(expected, actual); + + // another simple path, reset, and then append: expected.equals(actual) should equal 'true' + actual = new NodePath(null, "/a/b"); + actual.reset(); + actual.append(expected); + assertEquals(expected, actual); + + // allocate less than DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR, reset, and then append: expected.equals(actual) should equal 'true' + StringBuilder pathStrBuilder = new StringBuilder(); + for (int i = 0; i < (NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR) - 1; i++) { + pathStrBuilder.append("/a"); + } + actual = new NodePath(null, pathStrBuilder.toString()); + actual.reset(); + actual.append(expected); + assertEquals(expected, actual); + + // allocate exactly DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR, reset, and then append: expected.equals(actual) should equal 'true' + pathStrBuilder = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR; i++) { + pathStrBuilder.append("/a"); + } + actual = new NodePath(null, pathStrBuilder.toString()); + actual.reset(); + actual.append(expected); + assertEquals(expected, actual); + + // allocate over DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR, reset, and then append: expected.equals(actual) should equal 'true' + pathStrBuilder = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR * 2; i++) { + pathStrBuilder.append("/a"); + } + actual = new NodePath(null, pathStrBuilder.toString()); + actual.reset(); + actual.append(expected); + assertEquals(expected, actual); + } + + @Test + public void equalsAfterResetAndAddComponent() throws QName.IllegalQNameException { + final String strPath = "/a/b/c"; + + final NodePath expected = new NodePath(null, strPath); + + // simple path, reset, and then append: expected.equals(actual) should equal 'true' + NodePath actual = new NodePath(null, "/a"); + actual.reset(); + addComponents(actual, strPath); + assertEquals(expected, actual); + + // another simple path, reset, and then append: expected.equals(actual) should equal 'true' + actual = new NodePath(null, "/a/b"); + actual.reset(); + addComponents(actual, strPath); + assertEquals(expected, actual); + + // allocate less than DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR, reset, and then append: expected.equals(actual) should equal 'true' + StringBuilder pathStrBuilder = new StringBuilder(); + for (int i = 0; i < (NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR) - 1; i++) { + pathStrBuilder.append("/a"); + } + actual = new NodePath(null, pathStrBuilder.toString()); + actual.reset(); + addComponents(actual, strPath); + assertEquals(expected, actual); + + // allocate exactly DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR, reset, and then append: expected.equals(actual) should equal 'true' + pathStrBuilder = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR; i++) { + pathStrBuilder.append("/a"); + } + actual = new NodePath(null, pathStrBuilder.toString()); + actual.reset(); + addComponents(actual, strPath); + assertEquals(expected, actual); + + // allocate over DEFAULT_NODE_PATH_SIZE * MAX_OVER_ALLOCATION_FACTOR, reset, and then append: expected.equals(actual) should equal 'true' + pathStrBuilder = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE * NodePath.MAX_OVER_ALLOCATION_FACTOR * 2; i++) { + pathStrBuilder.append("/a"); + } + actual = new NodePath(null, pathStrBuilder.toString()); + actual.reset(); + addComponents(actual, strPath); + assertEquals(expected, actual); + } + + private void addComponents(final NodePath destination, final String path) throws QName.IllegalQNameException { + final String[] components = path.split("/"); + for (int i = 0; i < components.length; i++) { + final String component = components[i]; + if (!component.isEmpty()) { + destination.addComponent(new QName(component)); + } + } + } } From 8157af2d30fe03b896b1417c66ff6fad53303cd6 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 13:11:27 +0100 Subject: [PATCH 04/11] [bugfix] Fix a bug in NodePath#equals(Object) where it was not correctly accounting for over-allocation of 'components' array --- .../main/java/org/exist/storage/NodePath.java | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index 48bdd7a6733..6e58d2a4139 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -303,10 +303,24 @@ private void init(@Nullable final Map namespaces, final String p @Override public boolean equals(final Object obj) { - if (obj != null && obj instanceof NodePath) { + if (this == obj) { + return true; + } + + if (obj instanceof NodePath) { final NodePath otherNodePath = (NodePath) obj; - return Arrays.equals(components, otherNodePath.components); + + // NOTE(AR) we cannot use Array.equals on the components of the NodePaths as they may be over-allocated! + if (pos == otherNodePath.pos) { + for (int i = 0; i < pos; i++) { + if (!components[i].equals(otherNodePath.components[i])) { + return false; + } + } + return true; + } } + return false; } From dfe45fc128455e6279c47ec97ea9b1012465b14e Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 13:26:23 +0100 Subject: [PATCH 05/11] [optimise] Optimise the memory allocation/free strategy used by NodePath#append(NodePath) --- .../main/java/org/exist/storage/NodePath.java | 18 ++++-- .../java/org/exist/storage/NodePathTest.java | 63 +++++++++++++++++++ 2 files changed, 76 insertions(+), 5 deletions(-) diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index 6e58d2a4139..a435a198866 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -90,11 +90,19 @@ public boolean includeDescendants() { } public void append(final NodePath other) { - // expand the array - final int newLength = pos + other.length(); - this.components = Arrays.copyOf(components, newLength); - System.arraycopy(other.components, 0, components, pos, other.length()); - this.pos = newLength; + // do we have enough space to accommodate the components from `other` + final int available = components.length - pos; + final int numOtherComponentsToAppend = other.length(); + if (available < numOtherComponentsToAppend) { + // we need more space, allocate a multiple of DEFAULT_NODE_PATH_SIZE + final int allocationFactor = (int) Math.ceil((numOtherComponentsToAppend - available + components.length) / ((float) DEFAULT_NODE_PATH_SIZE)); + final int newSize = allocationFactor * DEFAULT_NODE_PATH_SIZE; + this.components = Arrays.copyOf(components, newSize); + } + + // at this point we have enough space, append the components from `other` + System.arraycopy(other.components, 0, components, pos, numOtherComponentsToAppend); + this.pos += numOtherComponentsToAppend; } public void addComponent(final QName component) { diff --git a/exist-core/src/test/java/org/exist/storage/NodePathTest.java b/exist-core/src/test/java/org/exist/storage/NodePathTest.java index cd5e00b3714..8be9912446d 100644 --- a/exist-core/src/test/java/org/exist/storage/NodePathTest.java +++ b/exist-core/src/test/java/org/exist/storage/NodePathTest.java @@ -146,6 +146,69 @@ public void reset() { assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, path.componentsSize()); } + @Test + public void appendAllocationStrategy() { + // `expected` has less occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has no components, when appending `expected` to `actual` then `actual` should have less occupied components than DEFAULT_NODE_PATH_SIZE + StringBuilder strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE - 1; i++) { + strPath.append("/a"); + } + NodePath expected = new NodePath(null, strPath.toString()); + NodePath actual = new NodePath(); + actual.append(expected); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, actual.componentsSize()); + + // `expected` has less occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has the same number of components, when appending `expected` to `actual` then `actual` should now be twice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE - 1; i++) { + strPath.append("/a"); + } + expected = new NodePath(null, strPath.toString()); + actual = new NodePath(null, strPath.toString()); + actual.append(expected); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 2, actual.componentsSize()); + + // `expected` has exactly DEFAULT_NODE_PATH_SIZE occupied components, `actual` has no components, when appending `expected` to `actual` then `actual` should have less occupied components than DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + expected = new NodePath(null, strPath.toString()); + actual = new NodePath(); + actual.append(expected); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, actual.componentsSize()); + + // `expected` has exactly DEFAULT_NODE_PATH_SIZE occupied components, `actual` has the same number of components, when appending `expected` to `actual` then `actual` should now be twice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + expected = new NodePath(null, strPath.toString()); + actual = new NodePath(null, strPath.toString()); + actual.append(expected); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 2, actual.componentsSize()); + + // `expected` has more occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has no components, when appending `expected` to `actual` then `actual` should now be twice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i <= NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + expected = new NodePath(null, strPath.toString()); + actual = new NodePath(); + actual.append(expected); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 2, actual.componentsSize()); + + // `expected` has more occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has the same number of components, when appending `expected` to `actual` then `actual` should now be thrice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i <= NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + expected = new NodePath(null, strPath.toString()); + actual = new NodePath(null, strPath.toString()); + actual.append(expected); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 3, actual.componentsSize()); + } + @Test public void equalsAfterResetAndAppend() { final String strPath = "/a/b/c"; From f817198d8be3203e4acdfabde738c068d149ea58 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 13:31:40 +0100 Subject: [PATCH 06/11] [optimise] Optimise the memory allocation/free strategy used by NodePath#addComponent(QName) --- .../main/java/org/exist/storage/NodePath.java | 37 ++++++++---- .../java/org/exist/storage/NodePathTest.java | 58 +++++++++++++++++++ 2 files changed, 83 insertions(+), 12 deletions(-) diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index a435a198866..58323a3c826 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -33,8 +33,17 @@ /** + * Represents a Node Path. + * + * Internally the node path is held as an array of {@link QName} components. + * Upon construction the array of `components` is sized such that it will be exactly + * what is required to represent the Node Path. + * Mutation operations however will over-allocate the array as an optmisation, so that + * we need not allocate/free memory on every mutation operation, but only + * every {@link #DEFAULT_NODE_PATH_SIZE} operations. + * + * @author Adam Retter * @author wolf - * @author Adam Retter */ public class NodePath implements Comparable { @@ -91,14 +100,8 @@ public boolean includeDescendants() { public void append(final NodePath other) { // do we have enough space to accommodate the components from `other` - final int available = components.length - pos; final int numOtherComponentsToAppend = other.length(); - if (available < numOtherComponentsToAppend) { - // we need more space, allocate a multiple of DEFAULT_NODE_PATH_SIZE - final int allocationFactor = (int) Math.ceil((numOtherComponentsToAppend - available + components.length) / ((float) DEFAULT_NODE_PATH_SIZE)); - final int newSize = allocationFactor * DEFAULT_NODE_PATH_SIZE; - this.components = Arrays.copyOf(components, newSize); - } + allocateIfNeeded(numOtherComponentsToAppend); // at this point we have enough space, append the components from `other` System.arraycopy(other.components, 0, components, pos, numOtherComponentsToAppend); @@ -106,13 +109,23 @@ public void append(final NodePath other) { } public void addComponent(final QName component) { - if (pos == components.length) { - // extend the array - this.components = Arrays.copyOf(components, pos + 1); - } + // do we have enough space to add the component + allocateIfNeeded(1); + + // at this point we have enough space, add the component components[pos++] = component; } + private void allocateIfNeeded(final int numOtherComponentsToAppend) { + final int available = components.length - pos; + if (available < numOtherComponentsToAppend) { + // we need more space, allocate a multiple of DEFAULT_NODE_PATH_SIZE + final int allocationFactor = (int) Math.ceil((numOtherComponentsToAppend - available + components.length) / ((float) DEFAULT_NODE_PATH_SIZE)); + final int newSize = allocationFactor * DEFAULT_NODE_PATH_SIZE; + this.components = Arrays.copyOf(components, newSize); + } + } + /** * Remove the Last Component from the NodePath. * diff --git a/exist-core/src/test/java/org/exist/storage/NodePathTest.java b/exist-core/src/test/java/org/exist/storage/NodePathTest.java index 8be9912446d..7c47d79594c 100644 --- a/exist-core/src/test/java/org/exist/storage/NodePathTest.java +++ b/exist-core/src/test/java/org/exist/storage/NodePathTest.java @@ -21,6 +21,7 @@ */ package org.exist.storage; +import org.exist.dom.QName; import org.junit.Test; import static org.junit.Assert.*; @@ -209,6 +210,63 @@ public void appendAllocationStrategy() { assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 3, actual.componentsSize()); } + @Test + public void addComponentAllocationStrategy() throws QName.IllegalQNameException { + // `expected` has less occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has no components, when appending `expected` to `actual` then `actual` should have less occupied components than DEFAULT_NODE_PATH_SIZE + StringBuilder strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE - 1; i++) { + strPath.append("/a"); + } + NodePath actual = new NodePath(); + addComponents(actual, strPath.toString()); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, actual.componentsSize()); + + // `expected` has less occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has the same number of components, when appending `expected` to `actual` then `actual` should now be twice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE - 1; i++) { + strPath.append("/a"); + } + actual = new NodePath(null, strPath.toString()); + addComponents(actual, strPath.toString()); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 2, actual.componentsSize()); + + // `expected` has exactly DEFAULT_NODE_PATH_SIZE occupied components, `actual` has no components, when appending `expected` to `actual` then `actual` should have less occupied components than DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + actual = new NodePath(); + addComponents(actual, strPath.toString()); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE, actual.componentsSize()); + + // `expected` has exactly DEFAULT_NODE_PATH_SIZE occupied components, `actual` has the same number of components, when appending `expected` to `actual` then `actual` should now be twice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i < NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + actual = new NodePath(null, strPath.toString()); + addComponents(actual, strPath.toString()); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 2, actual.componentsSize()); + + // `expected` has more occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has no components, when appending `expected` to `actual` then `actual` should now be twice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i <= NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + actual = new NodePath(); + addComponents(actual, strPath.toString()); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 2, actual.componentsSize()); + + // `expected` has more occupied components than DEFAULT_NODE_PATH_SIZE, `actual` has the same number of components, when appending `expected` to `actual` then `actual` should now be thrice the size of DEFAULT_NODE_PATH_SIZE + strPath = new StringBuilder(); + for (int i = 0; i <= NodePath.DEFAULT_NODE_PATH_SIZE; i++) { + strPath.append("/a"); + } + actual = new NodePath(null, strPath.toString()); + addComponents(actual, strPath.toString()); + assertEquals(NodePath.DEFAULT_NODE_PATH_SIZE * 3, actual.componentsSize()); + } + @Test public void equalsAfterResetAndAppend() { final String strPath = "/a/b/c"; From d04d96e6be27a09e2a3a2420fbe45d88877d9669 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 13:35:17 +0100 Subject: [PATCH 07/11] [optimise] Use a power of 2 for the array size --- exist-core/src/main/java/org/exist/storage/NodePath.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exist-core/src/main/java/org/exist/storage/NodePath.java b/exist-core/src/main/java/org/exist/storage/NodePath.java index 58323a3c826..d887c8eb0f7 100644 --- a/exist-core/src/main/java/org/exist/storage/NodePath.java +++ b/exist-core/src/main/java/org/exist/storage/NodePath.java @@ -47,7 +47,7 @@ */ public class NodePath implements Comparable { - static final int DEFAULT_NODE_PATH_SIZE = 5; + static final int DEFAULT_NODE_PATH_SIZE = 4; static final int MAX_OVER_ALLOCATION_FACTOR = 2; private static final Logger LOG = LogManager.getLogger(NodePath.class); From dad1cac8c79f6de1a188dee611510c9e8c68c438 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 13:36:22 +0100 Subject: [PATCH 08/11] [bugfix] When looking at QName#toString() output, e.g. when debugging, make sure to show the namespace when there is no prefix --- exist-core/src/main/java/org/exist/dom/QName.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/exist-core/src/main/java/org/exist/dom/QName.java b/exist-core/src/main/java/org/exist/dom/QName.java index 5a675c004ca..8d9748d87ba 100644 --- a/exist-core/src/main/java/org/exist/dom/QName.java +++ b/exist-core/src/main/java/org/exist/dom/QName.java @@ -41,6 +41,8 @@ public class QName implements Comparable { public static final String WILDCARD = "*"; private static final char COLON = ':'; + private static final char LEFT_BRACE = '{'; + private static final char RIGHT_BRACE = '}'; public static final QName EMPTY_QNAME = new QName("", XMLConstants.NULL_NS_URI); public static final QName DOCUMENT_QNAME = EMPTY_QNAME; @@ -127,8 +129,10 @@ public byte getNameType() { } public String getStringValue() { - if (prefix != null && prefix.length() > 0) { + if (prefix != null && !prefix.isEmpty()) { return prefix + COLON + localPart; + } else if (namespaceURI != null && !XMLConstants.NULL_NS_URI.equals(namespaceURI)) { + return LEFT_BRACE + namespaceURI + RIGHT_BRACE + localPart; } return localPart; } From 7bf6c4012111b1bccaf1ddb3dc18cc6237857e03 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 16:32:58 +0100 Subject: [PATCH 09/11] [bugfix] Correct the difference between QName#getStringValue() and QName#toString() --- .../src/main/java/org/exist/dom/QName.java | 50 +++++++++++-------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/exist-core/src/main/java/org/exist/dom/QName.java b/exist-core/src/main/java/org/exist/dom/QName.java index 8d9748d87ba..8edfe148552 100644 --- a/exist-core/src/main/java/org/exist/dom/QName.java +++ b/exist-core/src/main/java/org/exist/dom/QName.java @@ -128,34 +128,44 @@ public byte getNameType() { return nameType; } + /** + * Get a string representation of this qualified name. + * + * Will either be of the format `local-name` or `prefix:local-name`. + * + * @return the string representation of this qualified name. + * */ public String getStringValue() { - if (prefix != null && !prefix.isEmpty()) { - return prefix + COLON + localPart; - } else if (namespaceURI != null && !XMLConstants.NULL_NS_URI.equals(namespaceURI)) { - return LEFT_BRACE + namespaceURI + RIGHT_BRACE + localPart; - } - return localPart; + return getStringRepresentation(false); } /** - * @deprecated Use for debugging purpose only, - * use {@link #getStringValue()} for production + * Get a string representation of this qualified name. + * + * Will either be of the format `local-name`, `prefix:local-name`, or `{namespace}local-name`. + * + * @return the string representation of this qualified name. */ @Override public String toString() { - //TODO : remove this copy of getStringValue() - return getStringValue(); - //TODO : replace by something like this - /* - if (prefix != null && prefix.length() > 0) + return getStringRepresentation(true); + } + + /** + * Get a string representation of this qualified name. + * + * @param showNsWithoutPrefix true if the namespace should be shown even when there is no prefix, false otherwise. + * When shown, it will be output using Clark notation, e.g. `{http://namespace}local-name`. + * + * @return the string representation of this qualified name. + */ + private String getStringRepresentation(final boolean showNsWithoutPrefix) { + if (prefix != null && !prefix.isEmpty()) { return prefix + COLON + localPart; - if (hasNamespace()) { - if (prefix != null && prefix.length() > 0) - return "{" + namespaceURI + "}" + prefix + COLON + localPart; - return "{" + namespaceURI + "}" + localPart; - } else - return localPart; - */ + } else if (showNsWithoutPrefix && namespaceURI != null && !XMLConstants.NULL_NS_URI.equals(namespaceURI)) { + return LEFT_BRACE + namespaceURI + RIGHT_BRACE + localPart; + } + return localPart; } /** From 48bc6cdb7fc823ad69f53878c96deac5625c96d1 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 18:04:26 +0100 Subject: [PATCH 10/11] [bugfix] Fix an issue in the XQuery Inspection Module whereby QName#toString() was being called instead of QName#getStringValue() --- .../xquery/functions/inspect/InspectFunctionHelper.java | 6 +++--- .../org/exist/xquery/functions/inspect/InspectModule.java | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectFunctionHelper.java b/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectFunctionHelper.java index 8f5fe2b26ab..b0982ca9ac3 100644 --- a/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectFunctionHelper.java +++ b/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectFunctionHelper.java @@ -63,7 +63,7 @@ public static int generateDocs(final FunctionSignature sig, final UserDefinedFun XQDocHelper.parse(sig); final AttributesImpl attribs = new AttributesImpl(); - attribs.addAttribute("", "name", "name", "CDATA", sig.getName().toString()); + attribs.addAttribute("", "name", "name", "CDATA", sig.getName().getStringValue()); attribs.addAttribute("", "module", "module", "CDATA", sig.getName().getNamespaceURI()); final int nodeNr = builder.startElement(FUNCTION_QNAME, attribs); writeParameters(sig, builder); @@ -130,7 +130,7 @@ private static void writeAnnotations(final FunctionSignature signature, final Me if (annots != null) { for (final Annotation annot : annots) { attribs.clear(); - attribs.addAttribute(null, "name", "name", "CDATA", annot.getName().toString()); + attribs.addAttribute(null, "name", "name", "CDATA", annot.getName().getStringValue()); attribs.addAttribute(null, "namespace", "namespace", "CDATA", annot.getName().getNamespaceURI()); builder.startElement(ANNOTATION_QNAME, attribs); final LiteralValue[] value = annot.getValue(); @@ -164,7 +164,7 @@ private static void generateDependencies(final UserDefinedFunction function, fin final AttributesImpl attribs = new AttributesImpl(); for (final FunctionSignature signature : signatures) { attribs.clear(); - attribs.addAttribute(null, "name", "name", "CDATA", signature.getName().toString()); + attribs.addAttribute(null, "name", "name", "CDATA", signature.getName().getStringValue()); attribs.addAttribute("", "module", "module", "CDATA", signature.getName().getNamespaceURI()); attribs.addAttribute("", "arity", "arity", "CDATA", Integer.toString(signature.getArgumentCount())); diff --git a/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectModule.java b/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectModule.java index af760ffc53b..c9862a6626f 100644 --- a/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectModule.java +++ b/exist-core/src/main/java/org/exist/xquery/functions/inspect/InspectModule.java @@ -117,7 +117,7 @@ public Sequence eval(final Sequence[] args, final Sequence contextSequence) thro // variables for (final VariableDeclaration var : externalModule.getVariableDeclarations()) { attribs.clear(); - attribs.addAttribute("", "name", "name", "CDATA", var.getName().toString()); + attribs.addAttribute("", "name", "name", "CDATA", var.getName().getStringValue()); final SequenceType type = var.getSequenceType(); if (type != null) { attribs.addAttribute("", "type", "type", "CDATA", Type.getTypeName(type.getPrimaryType())); From 23827b92436b03a3f817ce9d5fc585fbd5972d39 Mon Sep 17 00:00:00 2001 From: Adam Retter Date: Sun, 10 Sep 2023 18:51:59 +0100 Subject: [PATCH 11/11] [bugfix] Fix an issue in the XQuery function util:describe-function#1 whereby QName#toString() was being called instead of QName#getStringValue() --- .../java/org/exist/xquery/functions/util/DescribeFunction.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exist-core/src/main/java/org/exist/xquery/functions/util/DescribeFunction.java b/exist-core/src/main/java/org/exist/xquery/functions/util/DescribeFunction.java index e91c81523d1..343c86a06bb 100644 --- a/exist-core/src/main/java/org/exist/xquery/functions/util/DescribeFunction.java +++ b/exist-core/src/main/java/org/exist/xquery/functions/util/DescribeFunction.java @@ -199,7 +199,7 @@ private void writeAnnotations(FunctionSignature signature, MemTreeBuilder builde if (annots != null) { for (final Annotation annot : annots) { attribs.clear(); - attribs.addAttribute(null, "name", "name", "CDATA", annot.getName().toString()); + attribs.addAttribute(null, "name", "name", "CDATA", annot.getName().getStringValue()); attribs.addAttribute(null, "namespace", "namespace", "CDATA", annot.getName().getNamespaceURI()); builder.startElement(ANNOTATION_QNAME, attribs); final LiteralValue[] value = annot.getValue();