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

Generalize global constraint variables to module V: Printable.S in each analysis #473

Merged
merged 44 commits into from
Dec 6, 2021

Conversation

sim642
Copy link
Member

@sim642 sim642 commented Dec 1, 2021

Closes #469.

Changes

  1. Adds module V: Printable.S to each analysis spec, which will be the module used for global constraint variables for that analysis. Since it's abstract V.t can be anything, not necessarily varinfo.
  2. Adds 'v type parameter to ctx. This is necessary to make ctx.global and ctx.sideg correctly typed for that V.t.
  3. Rewrite MCP's globals handling to use "variants" instead of lists for both global variables and their values. Each variant is just a pair of the analysis ID (int) and the corresponding variable/value as Obj.t.
  4. Refactor some analyses to use non-varinfo globals: thread, threadJoins, arinc and extract_arinc.
  5. Require analyses to explicitly define V if they want to use any globals (including varinfo). DefaultSpec just uses an empty (variant) type, which means no globals can be used at all.
  6. Fix g2html to handle the changed XML where globals have just one analysis value, not all.

TODO

@sim642 sim642 added type-safety Type-safety improvements precision performance Analysis time, memory usage labels Dec 1, 2021
@michael-schwarz michael-schwarz self-requested a review December 1, 2021 11:31
@michael-schwarz
Copy link
Member

Looks good thus far! I have to say I am thoroughly impressed that this did not require a major overhaul of everything!

@sim642
Copy link
Member Author

sim642 commented Dec 1, 2021

Before (x-axis) and after (y-axis) on SoftwareSystems and ConcurrencySafety:
image

Disappointingly, the performance only improves in a handful of cases. Also there's no precision improvement (at least on a super high level in the form of better verdicts) on these benchmarks.

I suppose for the single-threaded programs, there shouldn't be a difference anyway because global constraint variables are not used. For multi-threaded programs, I suspect the difference is maybe small because whereever you read a global (from the base analysis variable) the privatization forces the reading of another global (from the mutex analysis variable), so the dependencies we have are still quite tightly coupled between the analyses.

EDIT: I forgot that we now have our benchmarking server publically accessible, so the Benchexec tables are here: https://goblint.cs.ut.ee/results/49-all-fast-gen-glob-after/results/.

@michael-schwarz
Copy link
Member

What would be interesting is if the number of globals that are widening points changed, the hypothesis being that less widening might be applied. But then this is super hard to measure because we now have a lot more globals than we did before.

Also, it would be interesting to see what happens with runtime and the number of variables and evaluations if one runs one of the larger multi-threaded programs (e.g. the traces or more traces bechmarks), or some of the other larger projects from our test repo with earlyglobs.

@sim642
Copy link
Member Author

sim642 commented Dec 2, 2021

Also, it would be interesting to see what happens with runtime and the number of variables and evaluations if one runs one of the larger multi-threaded programs (e.g. the traces or more traces bechmarks), or some of the other larger projects from our test repo with earlyglobs.

I ran the traces benchmarks before and after, didn't notice much of a difference there either (and the bigger programs were from sv-benchmarks anyway). I don't know about the single-threaded benchmarks in bench though.

It's also the case that the base analysis and privatizations still use varinfo variables for everything. I don't know yet if there are any real opportunities to use the new generalization there, because most of the additional global variable components (background lockset, write lockset, thread ID) still need to be universally quantified over, so the (nested) maps need to remain to make that possible.

@sim642
Copy link
Member Author

sim642 commented Dec 3, 2021

It's also the case that the base analysis and privatizations still use varinfo variables for everything. I don't know yet if there are any real opportunities to use the new generalization there, because most of the additional global variable components (background lockset, write lockset, thread ID) still need to be universally quantified over, so the (nested) maps need to remain to make that possible.

I have now also refactored all the privatizations at the top level at least. So no more MUTEX_GLOBAL_g and MUTEX_INITS varinfos any more. Similarly for base protection-based I separated the unprotected and protected globals (which as discussed on Slack can make thing non-terminatingly more precise). For per-mutex privatizations this generalization also allows the privatization to account for mutex offsets, which were previously just stripped for privatization, although the protecting locksets could have them.

Moreover, I replaced the global product domains for those things with Lift2s.

EDIT: I now went even further and also made the base analysis globals a variant of the privatization's globals or thread returns. This allowed getting rid of all the thread return handling that previously had to be in all the privatizations because they were completely in charge of base's globals. Also it allowed to get rid of to_varinfo etc on thread IDs altogether because they're now first-class citizens everywhere!

@sim642
Copy link
Member Author

sim642 commented Dec 6, 2021

I ran sv-benchmarks again after my futher refactorings and there is now a 3% improvement in the regression line compared to master:
image
(Full comparisons here: https://goblint.cs.ut.ee/results/50-all-fast-gen-glob-after-refactor/results/).

I'm not sure where this improvement exactly came from though because the changes were to privatizations, which only matter for concurrent benchmarks and even then we don't use any of the more expensive privatizations.

@sim642 sim642 merged commit b98271d into master Dec 6, 2021
@sim642 sim642 deleted the general-globals branch December 6, 2021 13:59
@sim642 sim642 added this to the v2.0.0 milestone Aug 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Analysis time, memory usage precision type-safety Type-safety improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize global constraint variables from varinfo
2 participants