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 some object allocations that are identified as expensive by allocation profiling #104

Merged
merged 3 commits into from
May 12, 2021

Conversation

kawamuray
Copy link
Contributor

I found some code in decaton core causes massive object allocations (in proportional to consumed records) that causes high GC pressure.

The most significant ones are String allocation in LoggingContext (TaskMetadata.toString()) but TaskRequest#id too. (which is used only if trace level logging is enabled)

In this PR I saved those expensive object allocations by making it lazy or by adding a new flag to opt-out LoggingContext when its not necessary.

  • Adds new config decaton.logging.mdc.enabled

@kawamuray kawamuray added the new feature Add a new feature label May 11, 2021
@kawamuray kawamuray requested a review from ocadaruma May 11, 2021 11:17
Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

Only left one comment, but almost looks good.

MDC.put(PARTITION_KEY, String.valueOf(request.topicPartition().partition()));
private final boolean enabled;

public LoggingContext(boolean enabled, String subscriptionId, TaskRequest request, TaskMetadata metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to keep current constructor to avoid unnecessary breaking change since this class is considered as public API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me follow that.

@kawamuray kawamuray requested a review from ocadaruma May 12, 2021 02:56
Copy link
Contributor

@ocadaruma ocadaruma left a comment

Choose a reason for hiding this comment

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

LGTM!

@ocadaruma ocadaruma merged commit d2c99d8 into line:master May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants