You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by "nfsantos (via GitHub)" <gi...@apache.org> on 2025/02/24 14:37:19 UTC
[PR] Aggregate.getElementNameIfNotAPattern() is creating a String represen… [jackrabbit-oak]
nfsantos opened a new pull request, #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113
…tation of the elements arrays every time it is called, even though it was only needed if the checkArgument call failed. Replace this call by a conditional that avoids this expensive conversion to String.
In calls to collectAggregates, avoid the creation of ArrayList<Matcher> instances, instead use Matcher[]. Iteration over arrays with the enhanced for loop are converted to indexed traversals, which avoids the creation of an Iterator object while preserving the clean syntax of the enhanced for loop.
Replace usages of static constructor methods from Collections by corresponding methods in Set and Map.
In collectAggregatesForDirectMatchers(), do not create temporary children map and do not call matchChildren() unless there is a match among the children.
Fix white space.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975200084
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -132,82 +126,90 @@ private static boolean matchingType(String nodeTypeName, NodeState nodeState) {
return false;
}
- private static void collectAggregates(NodeState nodeState, List<Matcher> matchers,
- ResultCollector collector) {
- if (hasPatternMatcher(matchers)){
+ private static void collectAggregates(NodeState nodeState, Matcher[] matchers, ResultCollector collector) {
+ if (hasPatternMatcher(matchers)) {
collectAggregatesForPatternMatchers(nodeState, matchers, collector);
} else {
collectAggregatesForDirectMatchers(nodeState, matchers, collector);
}
}
- private static void collectAggregatesForDirectMatchers(NodeState nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForDirectMatchers(NodeState nodeState, Matcher[] matchers,
ResultCollector collector) {
- Map<String, ChildNodeEntry> children = new HashMap<>();
+ Map<String, ChildNodeEntry> children = null;
//Collect potentially matching child nodestates based on matcher name
- for (Matcher m : matchers){
+ for (Matcher m : matchers) {
String nodeName = m.getNodeName();
NodeState child = nodeState.getChildNode(nodeName);
- if (child.exists()){
+ if (child.exists()) {
+ if (children == null) {
+ children = new HashMap<>();
+ }
children.put(nodeName, new MemoryChildNodeEntry(nodeName, child));
}
}
- matchChildren(matchers, collector, children.values());
+ if (children != null) {
+ matchChildren(matchers, collector, children.values());
+ }
}
- private static void collectAggregatesForPatternMatchers(NodeState nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForPatternMatchers(NodeState nodeState, Matcher[] matchers,
ResultCollector collector) {
matchChildren(matchers, collector, nodeState.getChildNodeEntries());
}
- private static void matchChildren(List<Matcher> matchers, ResultCollector collector,
+ private static void matchChildren(Matcher[] matchers, ResultCollector collector,
Iterable<? extends ChildNodeEntry> children) {
+ List<Matcher> nextSet = null;
for (ChildNodeEntry cne : children) {
- List<Matcher> nextSet = new ArrayList<>(matchers.size());
for (Matcher m : matchers) {
Matcher result = m.match(cne.getName(), cne.getNodeState());
- if (result.getStatus() == Matcher.Status.MATCH_FOUND){
+ if (result.getStatus() == Matcher.Status.MATCH_FOUND) {
result.collectResults(collector);
}
- if (result.getStatus() != Matcher.Status.FAIL){
- nextSet.addAll(result.nextSet());
+ if (result.getStatus() != Matcher.Status.FAIL) {
+ if (nextSet == null) {
+ nextSet = new ArrayList<>();
+ }
+ result.nextSet(nextSet);
}
}
- if (!nextSet.isEmpty()) {
- collectAggregates(cne.getNodeState(), nextSet, collector);
+ if (nextSet !=null && !nextSet.isEmpty()) {
+ collectAggregates(cne.getNodeState(), nextSet.toArray(new Matcher[0]), collector);
+ nextSet.clear();
Review Comment:
Good point, I added some comments to explain the intent of this optimization. Let me know if it is clear.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "alagodasii (via GitHub)" <gi...@apache.org>.
alagodasii commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1973184799
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -588,37 +588,36 @@ public Matcher match(String name, NodeState nodeState) {
}
}
- public Collection<Matcher> nextSet() {
+ public List<Matcher> nextSet() {
checkArgument(status != Status.FAIL);
- if (status == Status.MATCH_FOUND){
+ if (status == Status.MATCH_FOUND) {
Aggregate nextAgg = currentInclude.getAggregate(matchedNodeState);
- if (nextAgg != null){
+ if (nextAgg != null) {
int recursionLevel = aggregateStack.size() + 1;
- if (recursionLevel >= rootState.rootAggregate.reAggregationLimit){
- return Collections.emptyList();
+ if (recursionLevel >= rootState.rootAggregate.reAggregationLimit) {
+ return List.of();
}
List<Matcher> result = new ArrayList<>(nextAgg.includes.size());
- for (Include i : nextAgg.includes){
- result.add(new Matcher(this, i, currentPath));
+ for (Include i : nextAgg.includes) {
+ result.add(new Matcher(this, i, currentPath));
}
return result;
}
- return Collections.emptyList();
+ return List.of();
Review Comment:
Actually, what's the benefit of using `List.of()` in comparison to `emptyList()`? Both create immutable lists without elements.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975200084
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -132,82 +126,90 @@ private static boolean matchingType(String nodeTypeName, NodeState nodeState) {
return false;
}
- private static void collectAggregates(NodeState nodeState, List<Matcher> matchers,
- ResultCollector collector) {
- if (hasPatternMatcher(matchers)){
+ private static void collectAggregates(NodeState nodeState, Matcher[] matchers, ResultCollector collector) {
+ if (hasPatternMatcher(matchers)) {
collectAggregatesForPatternMatchers(nodeState, matchers, collector);
} else {
collectAggregatesForDirectMatchers(nodeState, matchers, collector);
}
}
- private static void collectAggregatesForDirectMatchers(NodeState nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForDirectMatchers(NodeState nodeState, Matcher[] matchers,
ResultCollector collector) {
- Map<String, ChildNodeEntry> children = new HashMap<>();
+ Map<String, ChildNodeEntry> children = null;
//Collect potentially matching child nodestates based on matcher name
- for (Matcher m : matchers){
+ for (Matcher m : matchers) {
String nodeName = m.getNodeName();
NodeState child = nodeState.getChildNode(nodeName);
- if (child.exists()){
+ if (child.exists()) {
+ if (children == null) {
+ children = new HashMap<>();
+ }
children.put(nodeName, new MemoryChildNodeEntry(nodeName, child));
}
}
- matchChildren(matchers, collector, children.values());
+ if (children != null) {
+ matchChildren(matchers, collector, children.values());
+ }
}
- private static void collectAggregatesForPatternMatchers(NodeState nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForPatternMatchers(NodeState nodeState, Matcher[] matchers,
ResultCollector collector) {
matchChildren(matchers, collector, nodeState.getChildNodeEntries());
}
- private static void matchChildren(List<Matcher> matchers, ResultCollector collector,
+ private static void matchChildren(Matcher[] matchers, ResultCollector collector,
Iterable<? extends ChildNodeEntry> children) {
+ List<Matcher> nextSet = null;
for (ChildNodeEntry cne : children) {
- List<Matcher> nextSet = new ArrayList<>(matchers.size());
for (Matcher m : matchers) {
Matcher result = m.match(cne.getName(), cne.getNodeState());
- if (result.getStatus() == Matcher.Status.MATCH_FOUND){
+ if (result.getStatus() == Matcher.Status.MATCH_FOUND) {
result.collectResults(collector);
}
- if (result.getStatus() != Matcher.Status.FAIL){
- nextSet.addAll(result.nextSet());
+ if (result.getStatus() != Matcher.Status.FAIL) {
+ if (nextSet == null) {
+ nextSet = new ArrayList<>();
+ }
+ result.nextSet(nextSet);
}
}
- if (!nextSet.isEmpty()) {
- collectAggregates(cne.getNodeState(), nextSet, collector);
+ if (nextSet !=null && !nextSet.isEmpty()) {
+ collectAggregates(cne.getNodeState(), nextSet.toArray(new Matcher[0]), collector);
+ nextSet.clear();
Review Comment:
Good point, I added some comments to explain the intent of this optimization.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1973404886
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -588,37 +588,36 @@ public Matcher match(String name, NodeState nodeState) {
}
}
- public Collection<Matcher> nextSet() {
+ public List<Matcher> nextSet() {
checkArgument(status != Status.FAIL);
- if (status == Status.MATCH_FOUND){
+ if (status == Status.MATCH_FOUND) {
Aggregate nextAgg = currentInclude.getAggregate(matchedNodeState);
- if (nextAgg != null){
+ if (nextAgg != null) {
int recursionLevel = aggregateStack.size() + 1;
- if (recursionLevel >= rootState.rootAggregate.reAggregationLimit){
- return Collections.emptyList();
+ if (recursionLevel >= rootState.rootAggregate.reAggregationLimit) {
+ return List.of();
}
List<Matcher> result = new ArrayList<>(nextAgg.includes.size());
- for (Include i : nextAgg.includes){
- result.add(new Matcher(this, i, currentPath));
+ for (Include i : nextAgg.includes) {
+ result.add(new Matcher(this, i, currentPath));
}
return result;
}
- return Collections.emptyList();
+ return List.of();
Review Comment:
Several reasons:
- `List.of()` is a more modern API and seems to be the preferred choice for new APIs.
- It reads better and is more maintainable when used with the other static factory constructors in the List class:
```
List.of()
List.of("a")
List.of(1,2,3)
```
If we want to change between empty and non-empty, it is easier. And is more consistent.
- Performance may also be better is using only List.of() for every list used in a particular code path. This is because List.of() will return only 2 different implementations of the List class, which will mean that any call site that uses these objects will be mono or bimorphic. The JIT compiler can optimize these call sites to have similar performance as static calls. However, if there are more than 2 types of lists, the call site becomes polimorphic and performance is worse. Usually, this will have no measurable difference in performance, but in performance critical loops like here, the impact can be significant.
https://stackoverflow.com/questions/33317720/performance-of-collections-emptylist-and-empty-arraylist-with-jit-compiler
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975195679
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -588,37 +588,36 @@ public Matcher match(String name, NodeState nodeState) {
}
}
- public Collection<Matcher> nextSet() {
+ public List<Matcher> nextSet() {
checkArgument(status != Status.FAIL);
- if (status == Status.MATCH_FOUND){
+ if (status == Status.MATCH_FOUND) {
Aggregate nextAgg = currentInclude.getAggregate(matchedNodeState);
- if (nextAgg != null){
+ if (nextAgg != null) {
int recursionLevel = aggregateStack.size() + 1;
- if (recursionLevel >= rootState.rootAggregate.reAggregationLimit){
- return Collections.emptyList();
+ if (recursionLevel >= rootState.rootAggregate.reAggregationLimit) {
+ return List.of();
}
List<Matcher> result = new ArrayList<>(nextAgg.includes.size());
- for (Include i : nextAgg.includes){
- result.add(new Matcher(this, i, currentPath));
+ for (Include i : nextAgg.includes) {
+ result.add(new Matcher(this, i, currentPath));
}
return result;
}
- return Collections.emptyList();
+ return List.of();
Review Comment:
The readable improvement is mainly to use a uniform way of creating immutable lists. I think this is nicer:
```
List.of()
List.of("a")
List.of(1,2,3)
```
than
```
Collections.emptyList()
List.of("a")
List.of(1,2,3)
```
The performance improvements of bimorphic vs polimorphic calls will likely not be measurable in this case. However, as using `List.of` everywhere arguably makes the code more consistent and simpler, any minor gains are just a welcome extra.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "alagodasii (via GitHub)" <gi...@apache.org>.
alagodasii commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975053647
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -588,37 +588,36 @@ public Matcher match(String name, NodeState nodeState) {
}
}
- public Collection<Matcher> nextSet() {
+ public List<Matcher> nextSet() {
checkArgument(status != Status.FAIL);
- if (status == Status.MATCH_FOUND){
+ if (status == Status.MATCH_FOUND) {
Aggregate nextAgg = currentInclude.getAggregate(matchedNodeState);
- if (nextAgg != null){
+ if (nextAgg != null) {
int recursionLevel = aggregateStack.size() + 1;
- if (recursionLevel >= rootState.rootAggregate.reAggregationLimit){
- return Collections.emptyList();
+ if (recursionLevel >= rootState.rootAggregate.reAggregationLimit) {
+ return List.of();
}
List<Matcher> result = new ArrayList<>(nextAgg.includes.size());
- for (Include i : nextAgg.includes){
- result.add(new Matcher(this, i, currentPath));
+ for (Include i : nextAgg.includes) {
+ result.add(new Matcher(this, i, currentPath));
}
return result;
}
- return Collections.emptyList();
+ return List.of();
Review Comment:
For the readability reasons, `emptyList()` vs `List.of()` is arguable. Some could say, that "list of nothing" is less readable than "empty list of whatever". More interesting is that performance impact. I'm curious what would be the gain of performance after switch. Since, there are no performance tests in place, it would be worth to monitor executions and compare to historical ones.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "thomasmueller (via GitHub)" <gi...@apache.org>.
thomasmueller commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975033753
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -132,82 +126,90 @@ private static boolean matchingType(String nodeTypeName, NodeState nodeState) {
return false;
}
- private static void collectAggregates(NodeState nodeState, List<Matcher> matchers,
- ResultCollector collector) {
- if (hasPatternMatcher(matchers)){
+ private static void collectAggregates(NodeState nodeState, Matcher[] matchers, ResultCollector collector) {
+ if (hasPatternMatcher(matchers)) {
collectAggregatesForPatternMatchers(nodeState, matchers, collector);
} else {
collectAggregatesForDirectMatchers(nodeState, matchers, collector);
}
}
- private static void collectAggregatesForDirectMatchers(NodeState nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForDirectMatchers(NodeState nodeState, Matcher[] matchers,
ResultCollector collector) {
- Map<String, ChildNodeEntry> children = new HashMap<>();
+ Map<String, ChildNodeEntry> children = null;
//Collect potentially matching child nodestates based on matcher name
- for (Matcher m : matchers){
+ for (Matcher m : matchers) {
String nodeName = m.getNodeName();
NodeState child = nodeState.getChildNode(nodeName);
- if (child.exists()){
+ if (child.exists()) {
+ if (children == null) {
+ children = new HashMap<>();
+ }
children.put(nodeName, new MemoryChildNodeEntry(nodeName, child));
}
}
- matchChildren(matchers, collector, children.values());
+ if (children != null) {
+ matchChildren(matchers, collector, children.values());
+ }
}
- private static void collectAggregatesForPatternMatchers(NodeState nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForPatternMatchers(NodeState nodeState, Matcher[] matchers,
ResultCollector collector) {
matchChildren(matchers, collector, nodeState.getChildNodeEntries());
}
- private static void matchChildren(List<Matcher> matchers, ResultCollector collector,
+ private static void matchChildren(Matcher[] matchers, ResultCollector collector,
Iterable<? extends ChildNodeEntry> children) {
+ List<Matcher> nextSet = null;
for (ChildNodeEntry cne : children) {
- List<Matcher> nextSet = new ArrayList<>(matchers.size());
for (Matcher m : matchers) {
Matcher result = m.match(cne.getName(), cne.getNodeState());
- if (result.getStatus() == Matcher.Status.MATCH_FOUND){
+ if (result.getStatus() == Matcher.Status.MATCH_FOUND) {
result.collectResults(collector);
}
- if (result.getStatus() != Matcher.Status.FAIL){
- nextSet.addAll(result.nextSet());
+ if (result.getStatus() != Matcher.Status.FAIL) {
+ if (nextSet == null) {
+ nextSet = new ArrayList<>();
+ }
+ result.nextSet(nextSet);
}
}
- if (!nextSet.isEmpty()) {
- collectAggregates(cne.getNodeState(), nextSet, collector);
+ if (nextSet !=null && !nextSet.isEmpty()) {
+ collectAggregates(cne.getNodeState(), nextSet.toArray(new Matcher[0]), collector);
+ nextSet.clear();
Review Comment:
I had some trouble understanding why nextSet.clear() is needed... Yes, it is correct and matches the previous logic: before, a new ArrayList was created inside the the children loop. Now, this is outside the loop.
Yes, it is probably faster that way, but I'm worried that the logic is harder to understand, and in the future might cause some trouble... Not now... now the code is fine! But after some more changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos merged PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] OAK-11520 - Performance improvements to Aggregate class in indexing logic [jackrabbit-oak]
Posted by "nfsantos (via GitHub)" <gi...@apache.org>.
nfsantos commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975195679
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -588,37 +588,36 @@ public Matcher match(String name, NodeState nodeState) {
}
}
- public Collection<Matcher> nextSet() {
+ public List<Matcher> nextSet() {
checkArgument(status != Status.FAIL);
- if (status == Status.MATCH_FOUND){
+ if (status == Status.MATCH_FOUND) {
Aggregate nextAgg = currentInclude.getAggregate(matchedNodeState);
- if (nextAgg != null){
+ if (nextAgg != null) {
int recursionLevel = aggregateStack.size() + 1;
- if (recursionLevel >= rootState.rootAggregate.reAggregationLimit){
- return Collections.emptyList();
+ if (recursionLevel >= rootState.rootAggregate.reAggregationLimit) {
+ return List.of();
}
List<Matcher> result = new ArrayList<>(nextAgg.includes.size());
- for (Include i : nextAgg.includes){
- result.add(new Matcher(this, i, currentPath));
+ for (Include i : nextAgg.includes) {
+ result.add(new Matcher(this, i, currentPath));
}
return result;
}
- return Collections.emptyList();
+ return List.of();
Review Comment:
The readable improvement is mainly to use a uniform way of creating immutable lists. I think this is nicer:
```
List.of()
List.of("a")
List.of(1,2,3)
```
than
```
Collections.emptyList()
List.of("a")
List.of(1,2,3)
```
The performance improvements of bimorphic vs polimorphic calls in this case will likely not be measurable in this case. However, as using `List.of` everywhere arguably makes the code more consistent and simpler, any minor gains are just a welcome extra.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscribe@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org