-
Notifications
You must be signed in to change notification settings - Fork 34
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
Consolidating quantified field and predicate chunks #860
Consolidating quantified field and predicate chunks #860
Conversation
…ondition may have been changed
Hey, thanks a lot! To be honest, I don't think I was 100% certain that we never merge quantified chunks. We should definitely do that at least sometimes. |
I finally started looking into this, sorry this took so long.
I was really confused by this, but it really seems to be just an artifact of the resulting chunk order after a merge (even if the merge didn't merge anything), which just happens to be the perfect order for that example. I'll merge some recent changes into this (the debugging code I just added essentially required changes everywhere, so there's a huge amount of conflicts), do some benchmarks myself, and then merge this. |
Could you maybe send me one or two of your examples with lots of QP chunks that gets sped up by this? |
Here you go: https://gist.github.com/superaxander/b7a80e5de28cbb8a4597acf72387b782 |
Thanks! |
I noticed that state consolidation only merges basic chunks. I've been running into issues where if I have a lot of quantified field chunks things become very slow. In particular I have some files where a method verifies fine on it's own but it's welldefinedness can not be re-verified at the call site because the heap becomes too fragmented.
A similar issue is highlighted in #831 where if we're unlucky with the chunk ordering in removePermissions we have to "remove" some amount of permissions from a lot of unrelated chunks which obviously scales badly if we have a lot of chunks.
This PR enables merging quantified chunks (with the exception of magic wands) if their condition and arguments are the same. This seems to significantly speed up the above mentioned files (allowing the first example to verify and the example with 19 quantifiers from #831 now runs in 21 seconds) and not have a (significant) negative effect on other files. I've run the Silicon test suite and the VerCors test suite and saw no significant differences.
I did also try running Gobra on the router package from the VerifiedSCION repository which results in 1 error after ~30 minutes:
Since I cannot find another example where this PR has a negative impact and I don't know how to get a small Viper file from Gobra to test this specific case I thought I'd submit my PR anyway.
There are also still some open questions:
aloc
function anyway. Is it worth it to find a way to preserve these hints?forall i: Int :: 0 <= i && i < n ==> acc(aloc(x,i), write)
would yield a chunk with the condition0 <= i && i < n
and the permission valuewrite
, whereas the expressionforall i: Int :: true ==> acc(aloc(x, i), 0 <= i && i < n ? write : none)
would yield a chunk with the conditiontrue
and the permission value0 <= i && i < n ? write : none
. There is a trade-off here regarding when we do and don't want to merge chunks. In fact, if we conditionalize all the permissions (i.e. push the condition into the permission value) you could merge all chunks referring to the same field. But of course doing that would yield a single very complicated quantified heap chunk where we can't use heuristics to find a good chunk ordering anymore. Somewhere there is a good balance of merging chunks to simplify the state (i.e. instead of having two chunks containing half of the permission requiring 4 checks in removePermissions in the best case you can have one chunk containing all the permission requiring only 2 checks in the best case) and keeping chunks separate to enable reasoning about them separately.Let me know what you think of these changes and if I can improve them in some way