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

Replace PathCopyingPersistentTreeMap hash and size calculation #36

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Druidos
Copy link

@Druidos Druidos commented Mar 1, 2021

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
@PhilippWendler
Copy link
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
Copy link
Author

Druidos commented Mar 2, 2021

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.

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.

None yet

2 participants