Skip to content

Replace PathCopyingPersistentTreeMap hash and size calculation - #36

Closed
Druidos wants to merge 2 commits into
sosy-lab:mainfrom
Druidos:performance_fix_of_PathCopyingPersistentTreeMap
Closed

Druidos wants to merge 2 commits into
sosy-lab:mainfrom
Druidos:performance_fix_of_PathCopyingPersistentTreeMap

Conversation

@Druidos

@Druidos Druidos commented Mar 1, 2021

Copy link
Copy Markdown

Replace PathCopyingPersistentTreeMap hash and size calculation from lazy linear algorithm to pre-calculations, which is efficient for public copying methods

…azy linear algorithm to pre-calculations, which is efficient for public copying methods
@Druidos
Druidos requested a review from PhilippWendler March 1, 2021 16:51
@PhilippWendler

Copy link
Copy Markdown
Member

Thanks!

While this certainly increases the performance for some users, it causes higher memory usage for all users of this data structure due to the new fields in the node class. So I would like to understand the effects a little bit better before merging this.

Do you have any numbers on how much improves this performance or how much it increases memory usage in practice?
What is your use case that benefits from this? In CPAchecker, for example, we apply hash-code caching for these maps outside of them in those cases where we feel it is relevant, is this possible for you as well?

@Druidos

Druidos commented Mar 2, 2021

Copy link
Copy Markdown
Author

If memory consumption is critical, then I will move hash and size calculation from Node into PathCopyingPersistentTreeMap on put/remove-AndCopy.
Main idea is to optimize equals(). The hashcode calculation is obtained as a side result of the size calculation.
For example, following code twice iterates by whole pMap to identify size change:

boolean compare(PathCopyingPersistentTreeMap<Integer, Integer> pMap) {
    PathCopyingPersistentTreeMap<Integer, Integer> comp = pMap.putAndCopy(1,1);
    return comp.equals(pMap);
}

Exact numbers about performance I can provide later.

@PhilippWendler

Copy link
Copy Markdown
Member

I now realized one thing: Caching the hashCode() strictly speaking violates the Java API for map's hashCode(), because the hash code of a map depends on the hash code of all keys and values, and in particular the values need not be immutable and may change. Then the hash code of a map needs to reflect this.

Now, it is unlikely that someone would use a map with mutable values, actually change them, and then also depend on the map's hash code. More likely are cases where either the map's hash code is not used at all, or where the hash code is used but everything is immutable. But it could happen, and it would certainly lead to very confusing errors and costly debugging. So it would be better to not do this.

Tracking the size differences in the way that is discussed here would be possible, though. If someone wants to pick this up they can do so.

@PhilippWendler PhilippWendler linked an issue Sep 2, 2026 that may be closed by this pull request
@PhilippWendler
PhilippWendler removed their request for review September 2, 2026 06:18
@PhilippWendler

Copy link
Copy Markdown
Member

However, we need development to be done inside this repository such that we get CI results. So I am closing this PR, if work is taken up again, a new PR from an internal branch needs to be created.

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

Development

Successfully merging this pull request may close these issues.

Eager size tracking of PathCopyingPersistentTreeMap

2 participants