Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Profiler: Don’t profile NEXTDOC for ConstantScoreQuery. #33196

Merged
merged 6 commits into from
Sep 19, 2018
Merged

Profiler: Don’t profile NEXTDOC for ConstantScoreQuery. #33196

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ac73b0c
Profiler: Don’t profile NEXTDOC for ConstantScoreQuery.
iverase Aug 28, 2018
8060960
Override timings for constant score queries with timing of the underl…
iverase Sep 18, 2018
cf660f4
Merge branch with remote and resolve conflicts
iverase Sep 18, 2018
2875de0
fix forbidden API errors
iverase Sep 18, 2018
f2b0771
explain logic to find child scorer and make code safer by adding an i…
iverase Sep 18, 2018
88c8bca
add test that shows the different situations we can have with cached …
iverase Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/search/profile/AbstractProfileBreakdown.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ public Timer getTimer(T timing) {
return timings[timing.ordinal()];
}

public void setTimer(T timing, Timer timer) {
timings[timing.ordinal()] = timer;
}

/** Convert this record to a map from timingType to times. */
public Map<String, Long> toTimingMap() {
Map<String, Long> map = new HashMap<>();
Expand Down
22 changes: 22 additions & 0 deletions server/src/main/java/org/elasticsearch/search/profile/query/ProfileScorer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.search.profile.query;

import org.apache.lucene.search.ConstantScoreQuery;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.Scorer;
import org.apache.lucene.search.TwoPhaseIterator;
Expand All @@ -37,6 +38,7 @@ final class ProfileScorer extends Scorer {
private final Scorer scorer;
private ProfileWeight profileWeight;
private final Timer scoreTimer, nextDocTimer, advanceTimer, matchTimer;
private final boolean isConstantScoreQuery;

ProfileScorer(ProfileWeight w, Scorer scorer, QueryProfileBreakdown profile) throws IOException {
super(w);
Expand All @@ -46,6 +48,20 @@ final class ProfileScorer extends Scorer {
nextDocTimer = profile.getTimer(QueryTimingType.NEXT_DOC);
advanceTimer = profile.getTimer(QueryTimingType.ADVANCE);
matchTimer = profile.getTimer(QueryTimingType.MATCH);
ProfileScorer profileScorer = null;
if (w.getQuery() instanceof ConstantScoreQuery && scorer instanceof ProfileScorer) {
profileScorer = (ProfileScorer) scorer;
} else if (w.getQuery() instanceof ConstantScoreQuery && scorer.getChildren().size() == 1) {
profileScorer = (ProfileScorer) scorer.getChildren().iterator().next().child;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand when the if case may happen but I'm less sure about the second else if, when does it happen? Can you leave comments explaining?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, here is where the complexity is:

  1. The first if is hit when you have a totalHits query that is not cached. (Test testConstantScoreTotalHitsQuery and cache ==false).

  2. The else if is hit when you have a top N query. (Test testConstantScoreQuery)

  3. There is a third case where we have a totalHits query and it is cached. In this case the provided scorer is not an instance of ProfileScorer and has no children. In that case we do not do any special treatment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Can you add a comment to explain this? Also should we do an instanceof check on the scorer in the second case to be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a comment. Random test shows that the second check is always true but I will add a instanceof check to be on the safe side.

if (profileScorer != null) {
isConstantScoreQuery = true;
profile.setTimer(QueryTimingType.NEXT_DOC, profileScorer.nextDocTimer);
profile.setTimer(QueryTimingType.ADVANCE, profileScorer.advanceTimer);
profile.setTimer(QueryTimingType.MATCH, profileScorer.matchTimer);
} else {
isConstantScoreQuery = false;
}
}

@Override
Expand Down Expand Up @@ -75,6 +91,9 @@ public Collection<ChildScorer> getChildren() throws IOException {

@Override
public DocIdSetIterator iterator() {
if (isConstantScoreQuery) {
return scorer.iterator();
}
final DocIdSetIterator in = scorer.iterator();
return new DocIdSetIterator() {

Expand Down Expand Up @@ -112,6 +131,9 @@ public long cost() {

@Override
public TwoPhaseIterator twoPhaseIterator() {
if (isConstantScoreQuery) {
return scorer.twoPhaseIterator();
}
final TwoPhaseIterator in = scorer.twoPhaseIterator();
if (in == null) {
return null;
Expand Down
110 changes: 110 additions & 0 deletions server/src/test/java/org/elasticsearch/search/profile/query/QueryProfilerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.RandomIndexWriter;
import org.apache.lucene.index.Term;
import org.apache.lucene.search.ConstantScoreQuery;
import org.apache.lucene.search.Explanation;
import org.apache.lucene.search.IndexSearcher;
import org.apache.lucene.search.LeafCollector;
Expand Down Expand Up @@ -117,6 +118,115 @@ public void testBasic() throws IOException {
assertThat(rewriteTime, greaterThan(0L));
}

public void testConstantScoreQuery() throws IOException {
QueryProfiler profiler = new QueryProfiler();
searcher.setProfiler(profiler);
Query query = new ConstantScoreQuery(new TermQuery(new Term("foo", "bar")));
searcher.search(query, 1);
List<ProfileResult> results = profiler.getTree();
assertEquals(1, results.size());
Map<String, Long> breakdownConstantScoreQuery = results.get(0).getTimeBreakdown();
assertEquals(1, results.get(0).getProfiledChildren().size());
Map<String, Long> breakdownTermQuery = results.get(0).getProfiledChildren().get(0).getTimeBreakdown();

assertThat(breakdownConstantScoreQuery.get(QueryTimingType.CREATE_WEIGHT.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.BUILD_SCORER.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.ADVANCE.toString()).longValue(), equalTo(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.SCORE.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.MATCH.toString()).longValue(), equalTo(0L));

assertThat(breakdownConstantScoreQuery.get(QueryTimingType.CREATE_WEIGHT.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.BUILD_SCORER.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.NEXT_DOC.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.ADVANCE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.SCORE.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.MATCH.toString() + "_count").longValue(), equalTo(0L));

assertThat(breakdownTermQuery.get(QueryTimingType.CREATE_WEIGHT.toString()).longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString()).longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.ADVANCE.toString()).longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.SCORE.toString()).longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.MATCH.toString()).longValue(), equalTo(0L));

assertThat(breakdownTermQuery.get(QueryTimingType.CREATE_WEIGHT.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.ADVANCE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.SCORE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.MATCH.toString() + "_count").longValue(), equalTo(0L));

assertEquals(breakdownConstantScoreQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue());

long rewriteTime = profiler.getRewriteTime();
assertThat(rewriteTime, greaterThan(0L));
}

public void testConstantScoreTotalHitsQuery() throws IOException {
Query query = new ConstantScoreQuery(new TermQuery(new Term("foo", "bar")));

QueryProfiler profiler = new QueryProfiler();
searcher.setProfiler(profiler);
TotalHitCountCollector collector = new TotalHitCountCollector();
searcher.search(query, collector);

List<ProfileResult> results = profiler.getTree();
assertEquals(1, results.size());
Map<String, Long> breakdownConstantScoreQuery = results.get(0).getTimeBreakdown();
assertEquals(1, results.get(0).getProfiledChildren().size());
Map<String, Long> breakdownTermQuery = results.get(0).getProfiledChildren().get(0).getTimeBreakdown();

boolean cache = breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString()).longValue() ==0;

assertThat(breakdownConstantScoreQuery.get(QueryTimingType.CREATE_WEIGHT.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.BUILD_SCORER.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.ADVANCE.toString()).longValue(), equalTo(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.SCORE.toString()).longValue(), equalTo(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.MATCH.toString()).longValue(), equalTo(0L));

assertThat(breakdownConstantScoreQuery.get(QueryTimingType.CREATE_WEIGHT.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.BUILD_SCORER.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.NEXT_DOC.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.ADVANCE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.SCORE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownConstantScoreQuery.get(QueryTimingType.MATCH.toString() + "_count").longValue(), equalTo(0L));

assertThat(breakdownTermQuery.get(QueryTimingType.CREATE_WEIGHT.toString()).longValue(), greaterThan(0L));
if (cache) {
assertThat(breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString()).longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), equalTo(0L));
} else {
assertThat(breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString()).longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), greaterThan(0L));
}

assertThat(breakdownTermQuery.get(QueryTimingType.ADVANCE.toString()).longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.SCORE.toString()).longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.MATCH.toString()).longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.CREATE_WEIGHT.toString() + "_count").longValue(), greaterThan(0L));
if (cache) {
assertThat(breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString() + "_count").longValue(), equalTo(0L));
} else {
assertThat(breakdownTermQuery.get(QueryTimingType.BUILD_SCORER.toString() + "_count").longValue(), greaterThan(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString() + "_count").longValue(), greaterThan(0L));
}
assertThat(breakdownTermQuery.get(QueryTimingType.ADVANCE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.SCORE.toString() + "_count").longValue(), equalTo(0L));
assertThat(breakdownTermQuery.get(QueryTimingType.MATCH.toString() + "_count").longValue(), equalTo(0L));

if (cache == false) {
assertEquals(breakdownConstantScoreQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue(), breakdownTermQuery.get(QueryTimingType.NEXT_DOC.toString()).longValue());
}

long rewriteTime = profiler.getRewriteTime();
assertThat(rewriteTime, greaterThan(0L));

}


public void testNoScoring() throws IOException {
QueryProfiler profiler = new QueryProfiler();
searcher.setProfiler(profiler);
Expand Down