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

Removing volatile and synchronization in Text #12295

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Feb 16, 2025

Pull Request Description

While investigating slowdown of lengthOfStrings benchmark I came to conclusion that our Text implementation can be simplified:

  • no Lock
  • no volatile
  • use of switch (no semantic change)
  • use of record (no semantic change)

I don't see much need for locking or using volatile as

  • switching the value of contents field (and other simple fields) is always atomic
  • the old and the new values are both valid
  • the new value of contents field is String and String has all (important/non-idempotent) fields final
  • according to Java memory model final fields set in constructor are going to be properly visible from all threads

Hence I think we can do this without any synchronization.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Java,
  • Unit tests continue to work
  • Benchmarks are fine: engine run

@JaroslavTulach JaroslavTulach added the CI: Keep up to date Automatically update this PR to the latest develop. label Feb 21, 2025
@JaroslavTulach
Copy link
Member Author

StringBenchmarks.lengthOfString seems to be slightly improved

obrazek

which (I believe) is result of removing volatile.

@JaroslavTulach JaroslavTulach removed the CI: Keep up to date Automatically update this PR to the latest develop. label Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant