Skip to content

core: add average size and small frame percentage check for readable buffer#12904

Open
shivaspeaks wants to merge 8 commits into
grpc:masterfrom
shivaspeaks:prevent-heap-oom
Open

core: add average size and small frame percentage check for readable buffer#12904
shivaspeaks wants to merge 8 commits into
grpc:masterfrom
shivaspeaks:prevent-heap-oom

Conversation

@shivaspeaks

Copy link
Copy Markdown
Member

No description provided.

// mathematically equivalent to: percentage > 0.50
boolean isHighPercentageSmall = (smallFrameCount * 2) > size;

if (isAvgSmall || isHighPercentageSmall) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just inline the expressions so that it will skip evaluation of the 2nd argument to ||.

Comment thread core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java Outdated
}
}

private static final int MAX_QUEUED_BUFFERS = 1000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need some sort of load testing or data analysis to find optimal hyper params here? or maybe use the same values that other implementations use ?

@shivaspeaks shivaspeaks Jul 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not really sure on how to find the best constant here, it differs largely on usecases. Can we discuss this in the design? I don't have any plan for load testing right now but we can always tune it in the future if profiling dictates. My suggestion is to go with the current 1000 as it sounds reasonable.

Comment thread core/src/main/java/io/grpc/internal/CompositeReadableBuffer.java Outdated
@shivaspeaks shivaspeaks force-pushed the prevent-heap-oom branch 2 times, most recently from e2edacc to d9d0c06 Compare July 14, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants