Skip to content

[all] Include PubSubMessageHeaders size in ImmutablePubSubMessage heap size estimation #2287

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.

Sign up for GitHub

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

Open
lluwm wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[all] Include PubSubMessageHeaders size in ImmutablePubSubMessage heap size estimation #2287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af98823
[all] Include PubSubMessageHeaders size in ImmutablePubSubMessage hea…
lluwm Nov 14, 2025
4f9a823
increase buffercapacity in test
lluwm Nov 14, 2025
e10eec1
Merge branch 'main' into add_header_size
lluwm Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...inci-client/src/test/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTaskTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ public void suiteSetUp() throws Exception {
taskPollingService = Executors.newFixedThreadPool(1);
storeBufferService = new StoreBufferService(
3,
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

[nitpick] The buffer capacity increase from 10000 to 100000 (10x) appears to be a significant change for a test configuration. While this change may be necessary due to the more accurate heap size calculation now including headers, it would be helpful to add a comment explaining why this specific value was chosen and how it relates to the heap size estimation fix. This will help future maintainers understand the relationship between this test configuration and the heap size calculation change.

Suggested change
3,
3,
// Increased buffer capacity from 10,000 to 100,000 (10x) due to more accurate heap size calculation now including headers.
// This value ensures the test configuration remains valid with the heap size estimation fix.

Copilot uses AI. Check for mistakes.
10000,
100000,
1000,
isStoreWriterBufferAfterLeaderLogicEnabled(),
null,
Expand Down
3 changes: 2 additions & 1 deletion internal/venice-common/src/main/java/com/linkedin/venice/pubsub/ImmutablePubSubMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ public String toString() {
public int getHeapSize() {
/** The {@link #topicPartition} is supposed to be a shared instance, and is therefore ignored. */
return SHALLOW_CLASS_OVERHEAD + InstanceSizeEstimator.getObjectSize(key)
+ InstanceSizeEstimator.getObjectSize(value) + InstanceSizeEstimator.getObjectSize(pubSubPosition);
+ InstanceSizeEstimator.getObjectSize(value) + InstanceSizeEstimator.getObjectSize(pubSubPosition)
+ (pubSubMessageHeaders != null ? InstanceSizeEstimator.getObjectSize(pubSubMessageHeaders) : 0);
Copy link

Copilot AI Dec 8, 2025

Choose a reason for hiding this comment

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

The heap size calculation now includes pubSubMessageHeaders, but there are no tests specifically verifying that getHeapSize() correctly accounts for headers when they are present. Consider adding a test case in InstanceSizeEstimatorTest that creates an ImmutablePubSubMessage with headers (using the 7-parameter constructor) to ensure the heap size estimation is accurate when headers are included. This is important since the PR description indicates headers can be as large as 16KB and are now a significant contributor to memory usage.

Copilot uses AI. Check for mistakes.
}
}
Loading