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

Reduce memory usage in field-caps responses #88042

Merged
merged 5 commits into from
Jun 29, 2022

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Jun 26, 2022

We have reduced the memory usage of field-caps requests targeting many indices in 8.2+ (see #83494). Unfortunately, we still receive OOM reports in 7.17. I think we should push some contained improvements to reduce the memory usage for those requests in 7.17. I have looked into several options. This PR can reduce the memory usage of field-caps responses by replace HashMap with ArrayList for the field responses to eliminate duplicated string names and internal nodes of Map. I will look into other options, such as deduplicating field-name strings or field-responses.

@dnhatn dnhatn added v7.17.5 :Search/Search Search-related issues that do not fall into other categories >bug labels Jun 26, 2022
@dnhatn dnhatn marked this pull request as ready for review June 27, 2022 03:06
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jun 27, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

Hi @dnhatn, I've created a changelog YAML for you.

@javanna
Copy link
Member

javanna commented Jun 27, 2022

Heya @dnhatn, this change makes sense to me, good catch! Do you have numbers on the improvements when it comes to memory usage? Did you assess whether the change also has some effect on response time of field_caps?

@dnhatn
Copy link
Member Author

dnhatn commented Jun 28, 2022

@javanna Thanks for looking. This PR doesn't reduce the latency significantly as we still send a full map over the wire. The current version hits OOM with 10 concurrent field-caps requests, but this change doesn't.

@dnhatn dnhatn requested review from javanna and removed request for original-brownbear and jtibshirani June 28, 2022 03:16
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

Asked a question around testing, LGTM otherwise. The reason why I asked about numbers is that it would be nice to understand what to expect with this changes. Is it a 1%, 10% or 50% memory usage reduction approximately?

@dnhatn
Copy link
Member Author

dnhatn commented Jun 29, 2022

Is it a 1%, 10% or 50% memory usage reduction approximately?

I expect this PR reduces the memory usage by around 40%.

@dnhatn
Copy link
Member Author

dnhatn commented Jun 29, 2022

@javanna Thanks for review.

@dnhatn dnhatn merged commit 95b23fc into elastic:7.17 Jun 29, 2022
@dnhatn dnhatn deleted the hash_field_caps_response branch June 29, 2022 02:37
@lucabelluccini
Copy link
Contributor

This will be in 7.17.6.

@dnhatn dnhatn added v7.17.6 and removed v7.17.5 labels Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.17.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants