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.

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

Adds aggregation profiling to the profile API #18414

Merged
merged 1 commit into from Jun 10, 2016

Conversation

Projects
None yet
3 participants
@colings86
Copy link
Member

commented May 17, 2016

This change refactors some of the query profiling classes to extract common functionality that can be used to profile other features and then adds profiling of aggregations.

Caveats:

  • This is very early stage and definitely contains bugs
  • No tests exist yet for agg profiling
  • The reduce phase is not profiled yet and will always appear as 0. This is because we currently have no infrastructure on the coordinating node for profiling. This will probably be added in a later PR.

Relates to #10538

@colings86 colings86 self-assigned this May 17, 2016

@colings86 colings86 force-pushed the colings86:feature/aggProfiling branch 2 times, most recently May 17, 2016

@colings86 colings86 force-pushed the colings86:feature/aggProfiling branch 4 times, most recently May 31, 2016

@colings86 colings86 added the review label Jun 7, 2016

@colings86

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2016

@jpountz @polyfractal I've made some progress on this and it would be great to get your feedback on how close to ready you think it is.

@colings86 colings86 force-pushed the colings86:feature/aggProfiling branch Jun 7, 2016

@colings86 colings86 changed the title [Very WIP] Adds aggregation profiling to the profile API Adds aggregation profiling to the profile API Jun 7, 2016

@jpountz

View changes

core/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java Outdated
@@ -129,7 +129,8 @@ public boolean needsScores() {
@Override
public final LeafBucketCollector getLeafCollector(LeafReaderContext ctx) throws IOException {
final LeafBucketCollector sub = collectableSubAggregators.getLeafCollector(ctx);
return getLeafCollector(ctx, sub);
LeafBucketCollector leafCollector = getLeafCollector(ctx, sub);
return leafCollector;

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 7, 2016

Contributor

let's revert this change?

@jpountz

View changes

core/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactory.java Outdated

@Override
public void collect(int doc, long bucket) throws IOException {
aggregators = bigArrays.grow(aggregators, bucket + 1);

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 7, 2016

Contributor

should we move it under the if collector == null check to save some cycles in the common case?

@jpountz

View changes

core/src/main/java/org/elasticsearch/search/profile/AbstractInternalProfileTree.java Outdated

public AbstractInternalProfileTree() {
timings = new ArrayList<>(10);
stack = new LinkedBlockingDeque<>(10);

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 7, 2016

Contributor

hmm why do we need a blocking deque? a regular array deque should be enough?

@jpountz

View changes

core/src/main/java/org/elasticsearch/search/profile/AbstractInternalProfileTree.java Outdated
/** A list of top-level "roots". Each root can have its own tree of profiles */
protected ArrayList<Integer> roots;
/** A temporary stack used to record where we are in the dependency tree. Only used by scoring queries */
protected Deque<Integer> stack;

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 7, 2016

Contributor

then could it be only in the query impl?

This comment has been minimized.

Copy link
@colings86

colings86 Jun 7, 2016

Author Member

The comment was not updated after the refactoring to make this class abstract. This stack is used to record the position in the dependency tree and is needed for both the query and agg impls

@jpountz

View changes

core/src/main/java/org/elasticsearch/search/profile/aggregation/AggregationTimingType.java Outdated
import java.util.Locale;

public enum AggregationTimingType {
INITIALISE,

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 7, 2016

Contributor

nit-pick but I'm wondering that we might be using the american -ize in other parts of the response so we might want to be consistent?

@jpountz

View changes

core/src/main/java/org/elasticsearch/search/profile/aggregation/ProfilingAggregator.java Outdated

private Aggregator delegate;
private AggregationProfileBreakdown profileBreakdown;
private AggregationProfiler profiler;

This comment has been minimized.

Copy link
@jpountz

jpountz Jun 7, 2016

Contributor

can all fields be final?

This comment has been minimized.

Copy link
@colings86

colings86 Jun 7, 2016

Author Member

dleegate and profiler can be final but profileBreakdown is set in preCollection() so cannot be final.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2016

I like that this change is not too intrusive, the only hack I could spot is the getWrappedClass on the multi-bucket aggregator, which I think is fine. We might want to check whether we can make the profile wrappers pkg-private. Before giving an LGTM I would just like to do another iteration to better understand how the profile tree is built, but in general this looks on the right path to me.

@colings86

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2016

@jpountz thanks for the review. I've updated this PR with your comments. Note that the profile tree is the same logic as used for the query profiling.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2016

LGTM

@colings86 colings86 force-pushed the colings86:feature/aggProfiling branch Jun 10, 2016

Adds aggregation profiling (not including reduce phase)
Add Aggregation profiling initially only be for the shard phases (i.e. the reduce phase will not be profiled in this change)

This change refactors the query profiling class to extract abstract classes where it is useful for other profiler types to share code.

@colings86 colings86 force-pushed the colings86:feature/aggProfiling branch to 1d76177 Jun 10, 2016

@colings86 colings86 merged commit 62025d3 into elastic:master Jun 10, 2016

1 check passed

CLA Commit author has signed the CLA
Details

@colings86 colings86 deleted the colings86:feature/aggProfiling branch Jun 10, 2016

@clintongormley clintongormley removed the WIP label Jun 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.