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

Allocate memory lazily in BestBucketsDeferringCollector #43339

Merged

Conversation

@jimczi
Copy link
Member

commented Jun 18, 2019

While investigating memory consumption of deeply nested aggregations for #43091
the memory used to keep track of the doc ids and buckets in the BestBucketsDeferringCollector
showed up as one of the main contributor. In my tests half of the memory held in the
BestBucketsDeferringCollector is associated to segments that don't have matching docs
in the selected buckets. This is expected on fields that have a big cardinality since each
bucket can appear in very few segments. By allocating the builders lazily this change
reduces the memory consumption by a factor 2 (from 1GB to 512MB), hence reducing the
impact on gcs for these volatile allocations. This commit also switches the PackedLongValues.Builder
with a RoaringDocIdSet in order to handle very sparse buckets more efficiently.

I ran all my tests on the geonames rally track with the following query:

{
    "size": 0,
    "aggs": {
        "country_population": {
            "terms": {
                "size": 100,
                "field": "country_code.raw"
            },
            "aggs": {
                "admin1_code": {
                    "terms": {
                        "size": 100,
                        "field": "admin1_code.raw"
                    },
                    "aggs": {
                        "admin2_code": {
                            "terms": {
                                "size": 100,
                                "field": "admin2_code.raw"
                            },
                            "aggs": {
                                "sum_population": {
                                    "sum": {
                                        "field": "population"
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
While investigating memory consumption of deeply nested aggregations for #43091
the memory used to keep track of the doc ids and buckets in the BestBucketsDeferringCollector
showed up as one of the main contributor. In my tests half of the memory held in the
 BestBucketsDeferringCollector is associated to segments that don't have matching docs
 in the selected buckets. This is expected on fields that have a big cardinality since each
 bucket can appear in very few segments. By allocating the builders lazily this change
 reduces the memory consumption by a factor 2 (from 1GB to 512MB), hence reducing the
impact on gcs for these volatile allocations. This commit also switches the PackedLongValues.Builder
with a RoaringDocIdSet in order to handle very sparse buckets more efficiently.

I ran all my tests on the `geoname` rally track with the following query:

````
{
    "size": 0,
    "aggs": {
        "country_population": {
            "terms": {
                "size": 100,
                "field": "country_code.raw"
            },
            "aggs": {
                "admin1_code": {
                    "terms": {
                        "size": 100,
                        "field": "admin1_code.raw"
                    },
                    "aggs": {
                        "admin2_code": {
                            "terms": {
                                "size": 100,
                                "field": "admin2_code.raw"
                            },
                            "aggs": {
                                "sum_population": {
                                    "sum": {
                                        "field": "population"
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
````
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2019

Copy link
Contributor

left a comment

+1 to lazy allocation, can you leave a comment so that we don't disable this optimization by mistake? I'm surprised RoaringDocIdSet performs better, I'd actually expect it to require a bit more memory due to the fact it needs random-access capabilities.

@jimczi

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

I'm surprised RoaringDocIdSet performs better, I'd actually expect it to require a bit more memory due to the fact it needs random-access capabilities.

There was no real difference but I thought that RoaringDocIdSet would be a bit faster when deltas are big. I ran the benchmark again and the results are equivalent, however the RoaringDocIdSet doesn't handle duplicates so I reverted the change. I'll reevaluate this change in a follow up, the lazy allocation is enough for now to lower the memory usage.

jimczi added 2 commits Jun 18, 2019
Copy link
Member

left a comment

Neat! Had some comments about runtime speed and children aggs but looks like that's moot since you removed the Roaring stuff :)

Note to self: I wonder if the MergingBucketsDeferringCollector should get a similar treatment? It's nearly the same structure, just with addition code to handle merging buckets together. I suspect it's not a big deal given the only agg is the auto_date_histo. But rare_terms will use it... I can take a closer look and see if we can do the same thing :)

if (context == null) {
context = ctx;
docIdSetBuilder = new RoaringDocIdSet.Builder(context.reader().maxDoc());
bucketsBuilder = PackedLongValues.packedBuilder(PackedInts.DEFAULT);

This comment has been minimized.

Copy link
@polyfractal

polyfractal Jun 18, 2019

Member

Out of curiosity, have we tried COMPACT before to see how it affects memory usage and runtime speed?

This comment has been minimized.

Copy link
@jimczi

jimczi Jun 18, 2019

Author Member

I don't think so, at least I didn't ;)

DocIdSetIterator docIt = null;
if (needsScores && entry.docDeltas.size() > 0) {
DocIdSetIterator scoreIt = null;
if (needsScores) {

This comment has been minimized.

Copy link
@polyfractal

polyfractal Jun 18, 2019

Member

I'm assuming that since we're lazily creating these structures, we should never have an entry with empty docDeltasBuilder? Should we put an assert here to make sure?

This comment has been minimized.

Copy link
@jimczi

jimczi Jun 18, 2019

Author Member

I added a check in the Entry ctr: ecea58a

jimczi added 2 commits Jun 18, 2019
…ucketsDeferringCollector
@jimczi

This comment has been minimized.

Copy link
Member Author

commented Jun 18, 2019

I wonder if the MergingBucketsDeferringCollector should get a similar treatment? It's nearly the same structure, just with addition code to handle merging buckets together. I suspect it's not a big deal given the only agg is the auto_date_histo. But rare_terms will use it... I can take a closer look and see if we can do the same thing :

I pushed a change that refactors MergingBucketsDeferringCollector to be an extension of BestBucketsDeferringCollector in order to allocate the builders lazily in both cases:
ce7f447
WDYT ?

@polyfractal

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

Well that was easy enough, thanks @jimczi! Looks like it should integrate smoothly with the changes I have on my rare_terms branch as well.

@jimczi jimczi merged commit 5b1de3c into elastic:master Jun 19, 2019
8 checks passed
8 checks passed
CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details
@jimczi jimczi deleted the jimczi:enhancements/best_buckets_collector_lazy branch Jun 19, 2019
jimczi added a commit that referenced this pull request Jun 19, 2019
While investigating memory consumption of deeply nested aggregations for #43091
the memory used to keep track of the doc ids and buckets in the BestBucketsDeferringCollector
showed up as one of the main contributor. In my tests half of the memory held in the
 BestBucketsDeferringCollector is associated to segments that don't have matching docs
 in the selected buckets. This is expected on fields that have a big cardinality since each
 bucket can appear in very few segments. By allocating the builders lazily this change
 reduces the memory consumption by a factor 2 (from 1GB to 512MB), hence reducing the
impact on gcs for these volatile allocations. This commit also switches the PackedLongValues.Builder
with a RoaringDocIdSet in order to handle very sparse buckets more efficiently.

I ran all my tests on the `geoname` rally track with the following query:

````
{
    "size": 0,
    "aggs": {
        "country_population": {
            "terms": {
                "size": 100,
                "field": "country_code.raw"
            },
            "aggs": {
                "admin1_code": {
                    "terms": {
                        "size": 100,
                        "field": "admin1_code.raw"
                    },
                    "aggs": {
                        "admin2_code": {
                            "terms": {
                                "size": 100,
                                "field": "admin2_code.raw"
                            },
                            "aggs": {
                                "sum_population": {
                                    "sum": {
                                        "field": "population"
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
````
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Jun 27, 2019
While investigating memory consumption of deeply nested aggregations for elastic#43091
the memory used to keep track of the doc ids and buckets in the BestBucketsDeferringCollector
showed up as one of the main contributor. In my tests half of the memory held in the
 BestBucketsDeferringCollector is associated to segments that don't have matching docs
 in the selected buckets. This is expected on fields that have a big cardinality since each
 bucket can appear in very few segments. By allocating the builders lazily this change
 reduces the memory consumption by a factor 2 (from 1GB to 512MB), hence reducing the
impact on gcs for these volatile allocations. This commit also switches the PackedLongValues.Builder
with a RoaringDocIdSet in order to handle very sparse buckets more efficiently.

I ran all my tests on the `geoname` rally track with the following query:

````
{
    "size": 0,
    "aggs": {
        "country_population": {
            "terms": {
                "size": 100,
                "field": "country_code.raw"
            },
            "aggs": {
                "admin1_code": {
                    "terms": {
                        "size": 100,
                        "field": "admin1_code.raw"
                    },
                    "aggs": {
                        "admin2_code": {
                            "terms": {
                                "size": 100,
                                "field": "admin2_code.raw"
                            },
                            "aggs": {
                                "sum_population": {
                                    "sum": {
                                        "field": "population"
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.