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

PeristentHashMap.values creates a new instance of PersistentHashMapValues on each call #153

Open
develar opened this issue Oct 24, 2023 · 3 comments

Comments

@develar
Copy link

develar commented Oct 24, 2023

I don't know if it is an oversight or not, but fastutil/JDK caches computed values.

It is important for IntelliJ IDEA — PersistentMap.values called quite often.

I realize that modern JDK can optimize it, but it is still unclear to me whether it was benchmarked / by intention.

Screenshot 2023-10-24 at 18 03 19
intellij-monorepo-bot pushed a commit to JetBrains/intellij-community that referenced this issue Nov 1, 2023
known issue — Kotlin/kotlinx.collections.immutable#153 (still the change makes sense, as we do not create intermediate ArrayList)

GitOrigin-RevId: 821cafa2dffc173d19fd613e7e552f4fc5854add
intellij-monorepo-bot pushed a commit to JetBrains/intellij-community that referenced this issue Nov 2, 2023
known issue — Kotlin/kotlinx.collections.immutable#153 (still the change makes sense, as we do not create intermediate ArrayList)

(cherry picked from commit 821cafa2dffc173d19fd613e7e552f4fc5854add)

IJ-CR-117903

GitOrigin-RevId: 2116580985cd514c97ee0b94c1a53a241bc59c6f
@qurbonzoda
Copy link
Contributor

Hi,
This is done intentionally. The persistent collections are also immutable, and their internal state never changes after initialization. Therefor, caching is not possible.
Since PersistentMap.values/keys/entries returns a lightweight object, it was decided not to store it within the PersistentMap.

Thanks for the feedback! We really do appreciate it.

@qwwdfsad
Copy link
Member

qwwdfsad commented Feb 2, 2024

I would suggest re-opening the issue.

It might be the case that values reading is performance-sensitive and expected to be GC-free.
If we see that it's actually the case (e.g. any empirical evidence or a benchmark that it's a visible overhead), we'll reconsider that.

@qwwdfsad qwwdfsad reopened this Feb 2, 2024
@qurbonzoda
Copy link
Contributor

When considering the introduction of backing fields for the entries, keys, and values properties, an important factor should be taken into account. A persistent map is recreated with each mutation, and it is a common practice to "modify" an initial map repeatedly until reaching its final version. This process results in numerous intermediate, temporary maps, each incurring additional memory overhead. Therefore, special attention must be paid to minimizing this overhead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants