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

[improve][broker] Remove uneffective solution for reducing GC pressure #20428

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented May 29, 2023

Motivation

The PublisherStatsImpl and ConsumerStatsImpl classes contain an uneffective solution for reducing GC pressure.
This code is confusing and doesn't help. Instead, ordinary fields are used.
The solution was originally introduced by #1480.

Modifications

  • remove the uneffective solution
  • add a solution for reducing unnecessary object creation by sharing the client source address and port String object across all consumer and publisher stats on a single connection.

Additional Context

The Guava Interner based solution in #20432 could be a way to reduce GC pressure in a more effective way if there is a need for that.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added the type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use label May 29, 2023
@lhotari lhotari added this to the 3.1.0 milestone May 29, 2023
@lhotari lhotari self-assigned this May 29, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 29, 2023
@lhotari
Copy link
Member Author

lhotari commented May 29, 2023

/pulsarbot rerun-failure-checks

@JsonIgnore
private int producerNameLength;

private String producerName;
Copy link
Member

Choose a reason for hiding this comment

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

How can these changes affect JSON serde code path? I don't know if we pass or persist these classes via JSON somewhere.

If no, @JsonIgnore can be redundant. If so, adding new serialize field may affect especially this is a class under pulsar-common.

Copy link
Member Author

Choose a reason for hiding this comment

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

The former @JsonIgnore annotations were needed because of the extra ordinary solution to store String values. They aren't needed anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no change in JSON serialization and deserialization from an external viewpoint.

@lhotari lhotari requested a review from tisonkun May 31, 2023 13:21
@lhotari lhotari merged commit b336074 into apache:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test type/cleanup Code or doc cleanups e.g. remove the outdated documentation or remove the code no longer in use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants