Skip to content

Don't generate unnecessary fresh symbols for the GOTO trace #7021

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

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tautschnig
Copy link
Collaborator

We can safely record the values of expressions by adding expr == expr
as constraints in order to be able to fetch and display them in the GOTO
trace. This was already being done for declarations. Introducing new
symbols just adds unnecessary variables to the formula.

When running on various proofs done for AWS open-source projects, this
changes the performance as follows: with CaDiCaL as back-end, the total
solver time for the hardest 46 proofs changes from 26546.5 to 26779.7
seconds (233.2 seconds slow-down); with Minisat, however, the hardest 49
proofs take 28420.4 instead of 32387.2 seconds (3966.8 seconds
speed-up). Across these benchmarks, 1.7% of variables and 0.6% of
clauses are removed.

develop-vs-no-fresh

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@codecov
Copy link

codecov bot commented Jul 21, 2022

Codecov Report

Base: 78.26% // Head: 77.86% // Decreases project coverage by -0.39% ⚠️

Coverage data is based on head (30c9bce) compared to base (557624c).
Patch coverage: 95.83% of modified lines in pull request are covered.

❗ Current head 30c9bce differs from pull request most recent head 81afb57. Consider uploading reports for the commit 81afb57 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #7021      +/-   ##
===========================================
- Coverage    78.26%   77.86%   -0.40%     
===========================================
  Files         1642     1569      -73     
  Lines       189830   180969    -8861     
===========================================
- Hits        148568   140913    -7655     
+ Misses       41262    40056    -1206     
Impacted Files Coverage Δ
src/solvers/smt2/smt2_conv.cpp 68.78% <90.90%> (+2.39%) ⬆️
src/goto-symex/build_goto_trace.cpp 87.00% <100.00%> (-0.87%) ⬇️
src/goto-symex/symex_target_equation.cpp 95.04% <100.00%> (-0.34%) ⬇️
src/goto-programs/remove_unused_functions.cpp 0.00% <0.00%> (-100.00%) ⬇️
src/crangler/ctokenit.h 0.00% <0.00%> (-83.34%) ⬇️
src/crangler/ctokenit.cpp 0.00% <0.00%> (-82.76%) ⬇️
src/crangler/c_defines.cpp 44.00% <0.00%> (-48.00%) ⬇️
src/solvers/sat/cnf_clause_list.h 36.00% <0.00%> (-44.00%) ⬇️
src/solvers/sat/external_sat.cpp 57.14% <0.00%> (-30.96%) ⬇️
src/assembler/remove_asm.cpp 51.37% <0.00%> (-27.29%) ⬇️
... and 344 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tautschnig tautschnig force-pushed the cleanup/no-unnecessry-fresh-symbols branch from a0295ee to 537ebd6 Compare July 22, 2022 11:24
@tautschnig tautschnig marked this pull request as ready for review July 22, 2022 11:33
@tautschnig tautschnig force-pushed the cleanup/no-unnecessry-fresh-symbols branch from 537ebd6 to 5d892f4 Compare July 22, 2022 14:33
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Jul 22, 2022
Avoid creating equalities over the postponed bitvector when the object
literals trivially aren't equal, and directly encode bitwise equality
when the object literals are trivially equal (and stop searching for a
matching object). In all other cases, avoid unnecessary Tseitin
variables to encode the postponed bitvector equality.

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 22409.9 seconds (4369.8 seconds speed-up); with
Minisat, however, the hardest 49 proofs take 28616.7 instead of 28420.4
seconds (196.3 seconds slow-down). Across these benchmarks, 11.7% of
variables and 12.8% of clauses are removed.
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Aug 11, 2022
This was previously disabled as it appeared to degrade performance. New
benchmarking, however, suggests considerable performance improvement:

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 24472.6 seconds (2307.1 seconds speed-up); with
Minisat the hardest 49 proofs take 18541.2 instead of 28420.4
seconds (9879.2 seconds speed-up). Across these benchmarks, 1.0% of
variables and 3.2% of clauses are removed.
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Aug 11, 2022
This was previously disabled as it appeared to degrade performance. New
benchmarking, however, suggests considerable performance improvement:

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 24472.6 seconds (2307.1 seconds speed-up); with
Minisat the hardest 49 proofs take 18541.2 instead of 28420.4
seconds (9879.2 seconds speed-up). Across these benchmarks, 1.0% of
variables and 3.2% of clauses are removed.
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Aug 12, 2022
Avoid creating equalities over the postponed bitvector when the object
literals trivially aren't equal, and directly encode bitwise equality
when the object literals are trivially equal (and stop searching for a
matching object). In all other cases, avoid unnecessary Tseitin
variables to encode the postponed bitvector equality.

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 22409.9 seconds (4369.8 seconds speed-up); with
Minisat, however, the hardest 49 proofs take 28616.7 instead of 28420.4
seconds (196.3 seconds slow-down). Across these benchmarks, 11.7% of
variables and 12.8% of clauses are removed.
@tautschnig tautschnig force-pushed the cleanup/no-unnecessry-fresh-symbols branch from 5d892f4 to 30c9bce Compare August 12, 2022 11:58
@martin-cs
Copy link
Collaborator

I love the performance graphs but please can we have the diagonal line so it is easy to see better / worse.

Copy link
Collaborator

@martin-cs martin-cs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a little hard to follow how the changes implement what is described in the PR.

@tautschnig tautschnig assigned tautschnig and unassigned martin-cs Aug 13, 2022
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Aug 13, 2022
This was previously disabled as it appeared to degrade performance. New
benchmarking, however, suggests considerable performance improvement:

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 24472.6 seconds (2307.1 seconds speed-up); with
Minisat the hardest 49 proofs take 18541.2 instead of 28420.4
seconds (9879.2 seconds speed-up). Across these benchmarks, 1.0% of
variables and 3.2% of clauses are removed.
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Aug 14, 2022
This was previously disabled as it appeared to degrade performance. New
benchmarking, however, suggests considerable performance improvement:

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 24472.6 seconds (2307.1 seconds speed-up); with
Minisat the hardest 49 proofs take 18541.2 instead of 28420.4
seconds (9879.2 seconds speed-up). Across these benchmarks, 1.0% of
variables and 3.2% of clauses are removed.

INCLUDE_REDUNDANT_CLAUSES is not enabled: while this would yield a further
speed-up of 1080.4 seconds with CaDiCaL, it slows down Minisat by 4440.6
seconds on the above benchmark set.
tautschnig added a commit to tautschnig/cbmc that referenced this pull request Aug 14, 2022
This was previously disabled as it appeared to degrade performance. New
benchmarking, however, suggests considerable performance improvement:

When running on various proofs done for AWS open-source projects, this
changes the performance as follows (when comparing to diffblue#7021): with
CaDiCaL as back-end, the total solver time for the hardest 46 proofs
changes from 26779.7 to 24472.6 seconds (2307.1 seconds speed-up); with
Minisat the hardest 49 proofs take 18541.2 instead of 28420.4
seconds (9879.2 seconds speed-up). Across these benchmarks, 1.0% of
variables and 3.2% of clauses are removed.

INCLUDE_REDUNDANT_CLAUSES is not enabled: while this would yield a further
speed-up of 1080.4 seconds with CaDiCaL, it slows down Minisat by 4440.6
seconds on the above benchmark set.
@kroening
Copy link
Member

This is not a good idea.

I'd first like to understand how the speedup works. And then I'd like to push that change into the decision procedure. The user of the decision procedure (here symex) shouldn't have to care about encoding tricks that are this low level.

@tautschnig tautschnig marked this pull request as draft November 8, 2022 14:21
@tautschnig tautschnig force-pushed the cleanup/no-unnecessry-fresh-symbols branch 2 times, most recently from 57d6542 to 81afb57 Compare November 9, 2022 09:28
@tautschnig tautschnig changed the title Don't generate unnecessary fresh symbols for the GOTO trace Don't generate unnecessary fresh symbols for the GOTO trace [depends-on: #7310] Nov 9, 2022
@tautschnig tautschnig changed the title Don't generate unnecessary fresh symbols for the GOTO trace [depends-on: #7310] Don't generate unnecessary fresh symbols for the GOTO trace [depends-on: #7323] Nov 15, 2022
@tautschnig tautschnig changed the title Don't generate unnecessary fresh symbols for the GOTO trace [depends-on: #7323] Don't generate unnecessary fresh symbols for the GOTO trace Jan 11, 2023
We can safely record the values of expressions by adding `expr == expr`
as constraints in order to be able to fetch and display them in the GOTO
trace. This was already being done for declarations. Introducing new
symbols just adds unnecessary variables to the formula.

When running on various proofs done for AWS open-source projects, this
changes the performance as follows: with CaDiCaL as back-end, the total
solver time for the hardest 46 proofs changes from 26546.5 to 26779.7
seconds (233.2 seconds slow-down); with Minisat, however, the hardest 49
proofs take 28420.4 instead of 32387.2 seconds (3966.8 seconds
speed-up). Across these benchmarks, 1.7% of variables and 0.6% of
clauses are removed.
@tautschnig tautschnig force-pushed the cleanup/no-unnecessry-fresh-symbols branch from 81afb57 to c7f67c6 Compare July 24, 2024 14:36
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.

7 participants