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

Path: intern new Strings #432

Closed
wants to merge 1 commit into from

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Dec 12, 2023

When a filename is read from filesystem the time to intern() is not significant compared to the IO time but can save memory in the workspace

in case when i got an OOME it could have saved ~15MB
image

When a filename is read from filesystem the time to intern() is not
significant compared to the IO time but can save memory in the workspace
Copy link

Test Results

     24 files  ±0       24 suites  ±0   11m 27s ⏱️ ±0s
2 150 tests ±0  2 105 ✔️ ±0  45 💤 ±0  0 ±0 
2 194 runs  ±0  2 149 ✔️ ±0  45 💤 ±0  0 ±0 

Results for commit 0007c80. ± Comparison against base commit 9bb51e4.

@merks
Copy link
Contributor

merks commented Dec 12, 2023

I imagine that here, like with EMF's URIs, massive duplication of strings are involved!

Not that it's a huge concern, but are interned strings actually garage collectable in modern VMs? (Sorry if that was answered elsewhere already.)

@jukzi
Copy link
Contributor Author

jukzi commented Dec 12, 2023

are interned strings actually garage collectable in modern VMs

#33 (comment)
image

@jukzi
Copy link
Contributor Author

jukzi commented Dec 14, 2023

There are only known TCK fails. Mind to reviev/submit any commiter, please?

@iloveeclipse
Copy link
Member

Jörg, have you measured build time differences? We trade time vs memory here, and Path is heavily used during build.

@jukzi
Copy link
Contributor Author

jukzi commented Dec 14, 2023

Jörg, have you measured build time differences? We trade time vs memory here, and Path is heavily used during build.

good that you asked: i checked and stumbled on a very ineffective org.eclipse.core.internal.watson.ElementTreeIterator.requestPath() which uses String concatenation just to split them afterwards again, which used intern():
image

@jukzi jukzi marked this pull request as draft December 14, 2023 11:18
@jukzi
Copy link
Contributor Author

jukzi commented Dec 14, 2023

Let's drop this: it also interns ephemeral Paths
eclipse-platform/eclipse.platform@4e0f4a4 should be of much more help and we can investigate newer dumps for more potential - if needed at all.

@jukzi jukzi closed this Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants