thomasmueller commented on code in PR #2098:
URL: https://github.com/apache/jackrabbit-oak/pull/2098#discussion_r1965456655
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextIndexEditor.java:
##########
@@ -285,61 +296,73 @@ public void markDirty() {
}
private MatcherState getMatcherState(String name, NodeState after) {
- List<Aggregate.Matcher> matched = new ArrayList<>();
- List<Aggregate.Matcher> inherited = new ArrayList<>();
+ List<Aggregate.Matcher> matched = EMPTY_AGGREGATE_MATCHER_LIST;
+ List<Aggregate.Matcher> inherited = EMPTY_AGGREGATE_MATCHER_LIST;
for (Aggregate.Matcher m :
IterableUtils.chainedIterable(matcherState.inherited, currentMatchers)) {
Aggregate.Matcher result = m.match(name, after);
if (result.getStatus() == Aggregate.Matcher.Status.MATCH_FOUND) {
+ if (matched == EMPTY_AGGREGATE_MATCHER_LIST) {
+ matched = new ArrayList<>(4);
+ }
matched.add(result);
}
if (result.getStatus() != Aggregate.Matcher.Status.FAIL) {
+ if (inherited == EMPTY_AGGREGATE_MATCHER_LIST) {
+ inherited = new ArrayList<>(4);
+ }
inherited.addAll(result.nextSet());
}
}
- if (!matched.isEmpty() || !inherited.isEmpty()) {
+ if (matched.isEmpty() && inherited.isEmpty()) {
+ return MatcherState.NONE;
+ } else {
return new MatcherState(matched, inherited);
}
- return MatcherState.NONE;
}
+
/*
* Determines which all matchers are affected by this property change
*
* @param name modified property name
*/
private void checkAggregates(String name) {
- for (Aggregate.Matcher m : matcherState.matched) {
- if (!matcherState.affectedMatchers.contains(m)
- && m.aggregatesProperty(name)) {
- matcherState.affectedMatchers.add(m);
+ // Avoid allocating the iterator over matcherState.matched in the
common case of the list being empty
+ if (!matcherState.matched.isEmpty()) {
Review Comment:
I first thought there is no measurable performance impact in this additional
"if", but then I did measure and found that yes, indeed the additional
condition does help. For things that are called millions of times, this should
help a bit.
Unfortunately, Java doesn't support macros (custom "for" loops) like [some
other
languages](https://github.com/thomasmueller/bau-lang?tab=readme-ov-file#macros-and-ternary-condition)...
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextIndexEditor.java:
##########
@@ -285,61 +296,73 @@ public void markDirty() {
}
private MatcherState getMatcherState(String name, NodeState after) {
- List<Aggregate.Matcher> matched = new ArrayList<>();
- List<Aggregate.Matcher> inherited = new ArrayList<>();
+ List<Aggregate.Matcher> matched = EMPTY_AGGREGATE_MATCHER_LIST;
+ List<Aggregate.Matcher> inherited = EMPTY_AGGREGATE_MATCHER_LIST;
for (Aggregate.Matcher m :
IterableUtils.chainedIterable(matcherState.inherited, currentMatchers)) {
Aggregate.Matcher result = m.match(name, after);
if (result.getStatus() == Aggregate.Matcher.Status.MATCH_FOUND) {
+ if (matched == EMPTY_AGGREGATE_MATCHER_LIST) {
+ matched = new ArrayList<>(4);
+ }
matched.add(result);
}
if (result.getStatus() != Aggregate.Matcher.Status.FAIL) {
+ if (inherited == EMPTY_AGGREGATE_MATCHER_LIST) {
+ inherited = new ArrayList<>(4);
+ }
inherited.addAll(result.nextSet());
}
}
- if (!matched.isEmpty() || !inherited.isEmpty()) {
+ if (matched.isEmpty() && inherited.isEmpty()) {
+ return MatcherState.NONE;
+ } else {
return new MatcherState(matched, inherited);
}
- return MatcherState.NONE;
}
+
/*
* Determines which all matchers are affected by this property change
*
* @param name modified property name
*/
private void checkAggregates(String name) {
- for (Aggregate.Matcher m : matcherState.matched) {
- if (!matcherState.affectedMatchers.contains(m)
- && m.aggregatesProperty(name)) {
- matcherState.affectedMatchers.add(m);
+ // Avoid allocating the iterator over matcherState.matched in the
common case of the list being empty
+ if (!matcherState.matched.isEmpty()) {
+ for (Aggregate.Matcher m : matcherState.matched) {
+ if (!matcherState.affectedMatchers.contains(m) &&
m.aggregatesProperty(name)) {
+ matcherState.affectedMatchers.add(m);
+ }
}
}
}
- static class MatcherState {
+
+
+ public static class MatcherState {
final static MatcherState NONE = new MatcherState(List.of(),
List.of());
final List<Aggregate.Matcher> matched;
final List<Aggregate.Matcher> inherited;
final Set<Aggregate.Matcher> affectedMatchers;
- public MatcherState(List<Aggregate.Matcher> matched,
- List<Aggregate.Matcher> inherited) {
+ public MatcherState(List<Aggregate.Matcher> matched,
List<Aggregate.Matcher> inherited) {
this.matched = matched;
this.inherited = inherited;
//Affected matches would only be used when there are
//some matched matchers
if (matched.isEmpty()) {
- affectedMatchers = Collections.emptySet();
+ affectedMatchers = Set.of();
Review Comment:
I find it interesting to see that there is, in fact, a difference between
the two:
https://stackoverflow.com/questions/68759254/why-is-set-of-implemented-differently-from-collections-emptyset
```
Collections.emptySet().contains(null); // false
Set.of().contains(null); // NullPointerException
```
I don't think we rely on this specific difference, but it is still
interesting.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
