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

Reuse metric names in TopMetricsAggregator #116296

Merged
merged 6 commits into from
Nov 7, 2024

Conversation

iverase
Copy link
Contributor

@iverase iverase commented Nov 6, 2024

I noticed in a heapdump that we are creating a new list of names for each new InternalTopMetrics created which seems wasteful as we can be creating thousand of those objects during an aggregation execution. This commit shares a unique instance between all InternalTopMetrics instances.

@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Nov 6, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

@iverase iverase added >non-issue and removed v8.16.1 labels Nov 6, 2024
@iverase
Copy link
Contributor Author

iverase commented Nov 6, 2024

@elasticmachine update branch

@iverase
Copy link
Contributor Author

iverase commented Nov 6, 2024

@elasticmachine update branch

@iverase
Copy link
Contributor Author

iverase commented Nov 6, 2024

@elasticmachine update branch

@iverase
Copy link
Contributor Author

iverase commented Nov 6, 2024

@elasticmachine update branch

@iverase
Copy link
Contributor Author

iverase commented Nov 6, 2024

@elasticmachine update branch

Copy link
Contributor

@ivancea ivancea left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

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

LGTM.


Metrics(MetricValues[] values) {
this.values = values;
names = new ArrayList<>(values.length);
for (MetricValues value : values) {
Copy link
Member

Choose a reason for hiding this comment

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

I agree that the for loop is more readable here than the stream.

Copy link
Contributor Author

@iverase iverase Nov 7, 2024

Choose a reason for hiding this comment

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

And the stream was creating a list with initial size of 10 although we were adding only one element.

@iverase iverase added the auto-backport Automatically create backport pull requests when merged label Nov 7, 2024
@iverase iverase merged commit b1f2087 into elastic:main Nov 7, 2024
16 checks passed
@iverase iverase deleted the topmetricsnames branch November 7, 2024 14:15
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.x

iverase added a commit to iverase/elasticsearch that referenced this pull request Nov 7, 2024
This commit shares a unique instance between all InternalTopMetrics instances.
elasticsearchmachine pushed a commit that referenced this pull request Nov 7, 2024
This commit shares a unique instance between all InternalTopMetrics instances.
kderusso pushed a commit to kderusso/elasticsearch that referenced this pull request Nov 7, 2024
This commit shares a unique instance between all InternalTopMetrics instances.
jozala pushed a commit that referenced this pull request Nov 13, 2024
This commit shares a unique instance between all InternalTopMetrics instances.
alexey-ivanov-es pushed a commit to alexey-ivanov-es/elasticsearch that referenced this pull request Nov 28, 2024
This commit shares a unique instance between all InternalTopMetrics instances.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants