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] Replace String.intern() with Guava Interner #20432

Merged

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented May 29, 2023

Motivation

It is not recommended to use String.intern() at all. Guava contains a better replacement in it's Interner class.

Gradle build tool has been using Guava's interner for years (since it was introduced in 2015 with gradle/gradle@799e03e6) and it has been a successful solution in reducing String instance duplication without the problems that String.intern() has.

Modifications

Replace String.intern() with the singleton instance that uses Guava Interner

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 lhotari requested a review from merlimat May 29, 2023 13:50
@lhotari lhotari merged commit bdd1bf1 into apache:master May 29, 2023
@tisonkun
Copy link
Member

might cause memory leaks outside of DoS attacks since the instances will never be collected by GC

Can you provide evidence of this statement? I'm surprised if it's the case it should be a JDK bug. And the original PR introduced these lines #952 used it to reduce String objects.

@lhotari
Copy link
Member Author

lhotari commented May 30, 2023

might cause memory leaks outside of DoS attacks since the instances will never be collected by GC

Can you provide evidence of this statement? I'm surprised if it's the case it should be a JDK bug. And the original PR introduced these lines #952 used it to reduce String objects.

@tisonkun String intern seemed to have improved in JDK 8 and my knowledge about that was outdated. I was wrong about "never be collected by GC". Interned String instances will get collected as the last resort, in Full GC.
I'll remove the DoS argumentation from the PR description. However, as I mentioned, Guava's Interner is a very well optimized solution for interning objects. It has been used in Gradle since 2015 and it's very performant.

@tisonkun
Copy link
Member

Thanks for your explanation :D

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.

4 participants