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 and wire size of field caps internal responses #79119

Closed
wants to merge 1 commit into from

Conversation

dnhatn
Copy link
Member

@dnhatn dnhatn commented Oct 14, 2021

As the merging logic is too complex to introduce in 7x, I'd like to introduce a simpler and contained optimization that reduces the wire size and the memory usage of internal field cap responses (i.e., FieldCapabilitiesResponse between clusters and FieldCapabilitiesNodeResponse between nodes).

final boolean withOrdinals = responses.size() > 1;
output.writeBoolean(withOrdinals);
if (withOrdinals) {
final ObjectIntMap<String> dictionary = collectStringLiterals(responses);
Copy link
Member Author

@dnhatn dnhatn Oct 14, 2021

Choose a reason for hiding this comment

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

We can build this map incrementally when receiving/collecting responses.

@dnhatn
Copy link
Member Author

dnhatn commented Oct 14, 2021

@ywelsch @jimczi @jtibshirani Would you mind taking a look? Thank you!

@dnhatn dnhatn requested a review from ywelsch October 14, 2021 03:14
@jtibshirani
Copy link
Contributor

Instead of making a very specific optimization, would it make sense to look into enabling transport compression on these responses? I also wonder if we should build on @original-brownbear's recent PR to intern field types (#76405), instead of introducing a separate optimization? I guess that change only helps reduce the size on the deserialization side, it doesn't affect wire size.

@dnhatn
Copy link
Member Author

dnhatn commented Dec 9, 2021

We discussed this optimization and decided not to proceed with it. We prefer to have a new REST response where the merging logic uses smaller memory and can be scaled up.

@dnhatn dnhatn closed this Dec 9, 2021
@dnhatn dnhatn deleted the with-ordinals branch December 9, 2021 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants