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

Filter(s) aggregation should create weights only once. #15998

Merged
merged 1 commit into from Jan 15, 2016

Conversation

Projects
None yet
4 participants
@jpountz
Copy link
Contributor

commented Jan 14, 2016

We have a performance bug that if a filter aggregation is below a terms
aggregation that has a cardinality of 1000, we will call Query.createWeight
1000 times as well. However, Query.createWeight can be a costly operation.
For instance in the case of a TermQuery it will seek the term in every
segment. Instead, we should create the Weight once, and then get as many
iterators as we need from this Weight.

I found this problem while trying to diagnose a performance regression while
upgrading from 1.7 to 2.1[1]. While the problem was not introduced in 2.x, the
fact that 1.7 cached very aggressively had hidden this problem, since you don't
need to seek the term anymore on a cached TermFilter.

Doing things once for every aggregator is not easy with the current API but
I discussed this with Colin and Aggregator factories will need to get an init
method for different reasons, where we will be able to put these steps that
need to be performed only once, no matter haw many aggregators need to be
created.

[1] https://discuss.elastic.co/t/aggregations-in-2-1-0-much-slower-than-1-6-0/38056/26

@colings86

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

LGTM

@jasontedor

View changes

core/src/main/java/org/elasticsearch/search/aggregations/bucket/filters/FiltersAggregator.java Outdated
}

// TODO: refactor in order to initialize the factory once with its parent,
// the context, etc. and then have a no-arg lightweight create method
// (since create may be called thoulands of times)

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 14, 2016

Member

Typo: thoulands -> thousands. Same typo here.

@jasontedor

View changes

core/src/test/java/org/elasticsearch/search/aggregations/bucket/FilterIT.java Outdated
public void testAsSubAggregation() {
SearchResponse response = client().prepareSearch("idx")
.addAggregation(
histogram("histo").field("value").interval(2l).subAggregation(

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 14, 2016

Member

Nit: 2L is easier to read than 2l (because in some fonts it looks like 21). Same comment here. Sorry. :(

This comment has been minimized.

Copy link
@jpountz

jpountz Jan 14, 2016

Author Contributor

no need to be sorry!

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 14, 2016

Any opinions about whether to backport to 2.2?

Filter(s) aggregation should create weights only once.
We have a performance bug that if a filter aggregation is below a terms
aggregation that has a cardinality of 1000, we will call Query.createWeight
1000 times as well. However, Query.createWeight can be a costly operation.
For instance in the case of a TermQuery it will seek the term in every
segment. Instead, we should create the Weight once, and then get as many
iterators as we need from this Weight.

I found this problem while trying to diagnose a performance regression while
upgrading from 1.7 to 2.1[1]. While the problem was not introduced in 2.x, the
fact that 1.7 cached very aggressively had hidden this problem, since you don't
need to seek the term anymore on a cached TermFilter.

Doing things once for every aggregator is not easy with the current API but
I discussed this with Colin and Aggregator factories will need to get an init
method for different reasons, where we will be able to put these steps that
need to be performed only once, no matter haw many aggregators need to be
created.

[1] https://discuss.elastic.co/t/aggregations-in-2-1-0-much-slower-than-1-6-0/38056/26

@jpountz jpountz force-pushed the jpountz:fix/filter_agg_creates_meights_once branch to cc41e6e Jan 15, 2016

jpountz added a commit that referenced this pull request Jan 15, 2016

Merge pull request #15998 from jpountz/fix/filter_agg_creates_meights…
…_once

Filter(s) aggregation should create weights only once.

@jpountz jpountz merged commit 0806190 into elastic:master Jan 15, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jpountz jpountz deleted the jpountz:fix/filter_agg_creates_meights_once branch Jan 15, 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.