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

Share Name.Literal strings #11886

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Dec 17, 2024

Pull Request Description

Fixes #10330 by interning Name.Literal strings.

Checklist

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

  • All code follows the
    Scala,
    Java,
  • Unit tests have been written where possible.
  • Benchmarks are OK

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Dec 17, 2024
@JaroslavTulach JaroslavTulach self-assigned this Dec 17, 2024
Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

So what is the difference of memory consumption by all the Name.Literal objects on the heap? Is there any difference in sizes of caches?

@JaroslavTulach
Copy link
Member Author

Previously there was almost 6000 of strings representing Standard in memory when running test/Base_Tests:

5700 strings

now, after the fix we have just one:

one string

The overall amount of memory occupied by java.lang.String instances went down by 5MB.

@JaroslavTulach
Copy link
Member Author

Is there any difference in sizes of caches?

I don't see any difference in the size of caches. Probably because of

the intern() being already called before storing the strings.

@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Dec 17, 2024

Of course, calling String.intern() slows things down. Hopefully the showdown isn't going to affect our benchmarks:

Benchmarks don't seem to be affected by this change, but while investigating them I realized #11901. Anyway with 13db21c test we are ready to integrate.

@JaroslavTulach JaroslavTulach added CI: Ready to merge This PR is eligible for automatic merge CI: Clean build required CI runners will be cleaned before and after this PR is built. labels Dec 18, 2024
@mergify mergify bot merged commit e4a4bcc into develop Dec 18, 2024
45 checks passed
@mergify mergify bot deleted the wip/jtulach/ShareNameLiteralStrings10330 branch December 18, 2024 06:46
@enso-org enso-org deleted a comment from enso-bot bot Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too many duplicated Name.Literal instances
2 participants