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

Do not use vectors indexed by Pterms' ids in LASolver #500

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

blishko
Copy link
Member

@blishko blishko commented May 24, 2022

This PR suggests to replace the inner representation of how LASolver maintains mappings from Pterms to its inner representations (LVRefs and bounds).
Instead of vectors indexed by PTId, I suggest to use just normal HashMap.
The disadvantage of current representation is its memory inefficiency, because the size of the vectors possibly depends on the number of terms in Logic. But Logic may contain a huge number of terms compared to a few atoms that need to be represented in LASolver.

For OpenSMT itself this does not make much difference, but for Golem, it can have noticeable impact, as LASolver is re-initialized on every check-sat call, and Golem makes quite a lot of them.

@blishko blishko requested a review from aehyvari May 24, 2022 14:37
@aehyvari
Copy link
Member

Did you try it with qf_lra or qf_Lia already? I have some (probably outdated but still) evidence that hash maps might be a lot slower than an array.

@blishko
Copy link
Member Author

blishko commented May 24, 2022

No, I wanted to try, but the cluster was not working today. I'll try later.
I also am a little worried, but this is not really critical code, so I would not expect the performance on single-queries to be much different. But I will try at least QF_LRA and post the results.
I started considering writing a specialized hashmap, but I hope it will not be necessary :)

I have a benchmark for Golem where this change meant more than 10% improvement.

@aehyvari
Copy link
Member

OK let's wait for the results. One option is to run them in mantella, there are some scripts to do that and it might be faster than waiting for the cluster to come up.

Martin Blicha added 3 commits May 25, 2022 10:58
When LAVarMapper is storing mapping from PTerms to LATerms as vectors
with TermId as index, this creates possibly huge vectors with just a
small number of entries filled. This is not a problem for single-use
instances, but in applications where number of terms in Logic is growing
constantly are a solver is queries repeatedly, the initialization of
LASolver due to allocation of large vectors starts to be noticeable.

For now we use standard hashmap as a replacement. It seems that some
kind of flat map, which additioanlly does not need to support deletion
could be even better.
@blishko
Copy link
Member Author

blishko commented May 25, 2022

comparison_master_maps.pdf

It seems there is some slowdown associated with this.
Maybe I need implement custom hashmap :)

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

Successfully merging this pull request may close these issues.

2 participants