-
Notifications
You must be signed in to change notification settings - Fork 63
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
Solver result caching #1873
Solver result caching #1873
Conversation
To clarify for the curious: I used sqlite extensively at my previous job and from experience it gets a bit tricky when concurrency is involved. You'll notice that the page on when to use sqlite calls out high levels of concurrent writes as something sqlite does not handle well. In practice there are tricks to make this better (such as enabling WAL mode), but still any application that makes use of concurrent writes (or reads while writing) will need to handle sqlite errors that can arise at any point a query is made. Usually a simple "wait and try again" approach works, but ultimately it can take a few months of tweaks to get everything working well. Additionally, sqlite's performance can vary significantly depending on how you structure your tables and queries. We would probably want to take the time to design that out before implementing it. It's not that I think we shouldn't use a database for this at some point, I just wanted to push back on the idea that it's a quick change to use sqlite in a concurrent application like this. |
Can you say more about that What4 cache file that is next to the Z3 cache file? What4 tests currently do determine solver versions; it seems reasonable that this could be moved into the what4 library itself and surfaced somewhere in the API: https://github.com/GaloisInc/what4/blob/master/what4/test/ProbeSolvers.hs#L39 My opinion is that we definitely want a solver-specific cache (different solvers can give different answers) and a version-specific solver cache (does a new version of a solver still give the same answers?). |
Not sure exactly what you're asking, but currently the cache files all have the same format: the filename is the hash of the saw-core term which is passed to the solver backend, and the solver version is just saved as a string in that file.
Amazing - thanks for the link! I'm going to use this to quickly get a version where we also cache by solver version up and running and I'll update the example. |
Oh, I misread the original and thought you only had one query, but you generated two different queries from the |
Let's be clear about goals: the primary goal of this SMT caching feature is to speed up CI. It could maybe be useful in other contexts, but CI is the main use case of interest. Now, the easiest way to use this sort of cache in github CI is to use the github cache action, which gives each workflow its own local version of the cache. There doesn't seem to be a straightforward way to get different workflows to share a cache, and in fact different workflows will probably make different queries anyway on the whole, so separating the caches to different CI workflows is probably a good thing in the end. Thus, we only really have to worry about concurrency inside a single workflow. Some of our existing workflows are completely sequential, since they are just a single SAW file. But things like the Given all of this, I would say we only need to support a low degree of concurrency. SQLite seems feasible, but OTOH the one file per query approach seems intriguing as well, for the reasons you outlined. And in fact, the one file per query approach could actually have better performance, because most of the concurrent processes in a CI run will be making different SMT queries: the one file per query approach would allow these to happen concurrently, while SQLite would serialize the writes. It should even be straightforward to correctly handle the concurrency here: the My suggestion for SMT solver versions is this: each SMT cache should be globally marked when it is created with the versions of all the solvers it was created with. Whenever a CI workflow is started and the SMT cache is loaded / installed, some process should check those SMT solver versions against the current versions that are installed, and just wipe the cache if any of those solver versions have changed. The expectation is that we are only occasionally going to change SMT solver versions, and when we do, we don't expect to go back to an old version, so there is no reason to keep the old cached values around. I could imagine trying to only wipe the cached queries corresponding to the particular solver whose version was updated, but then you have to associate each query with the solver it was made against, and this creates additionally complexity. Again, we are only occasionally going to change SMT solver versions for any of our solvers, so we might as well save ourselves the trouble and just wipe the whole SMT cache. Then, for each copy of SAW that gets run in the workflow, the SMT caching component(s) will check if the solver versions it sees are the same as those given in the SMT cache. If not, then something is wrong with the CI setup. It should probably raise an error, though I could imagine cases where we want to turn that into a warning instead, though it should be a stern warning. :) If it's a warning and not an error, it should at least be something that the user can easily see when checking the CI logs. That is my 2 cents. Which buys a lot of text these days, apparently. |
Caching is now dependent on the solver used as well as the version of the solver used. Thanks @kquick for the pointer! I updated the example in the PR description, but here's the short of it:
I also updated the last question in the PR description, since now I have questions about the implementation I went with. |
I'm actually more in favor of including the SMT version in the file (and as part of the input to the hash for the file's name). I don't see it as a lot of additional work, and while the primary utilization of the cache is intended to be the CI, the caching is being implemented by saw-script itself and not the CI, so I think it should be robust against other uses. It's also worth noting that the what4 repo tests multiple different solver versions during its CI and that might be desirable for saw at some future point as well. |
Along with saving/hashing against solver version, I suggest also
saving/hashing against any command line options to the solver, in case they
also change.
Our CI scripts mostly use the Python interfaces though both saw-remote-api
and cryptol-remote-api, so it would be _great_ to give both python
interfaces access to these new caching options.
…On Fri, May 19, 2023 at 9:51 PM Kevin Quick ***@***.***> wrote:
My suggestion for SMT solver versions is this: each SMT cache should be
globally marked when it is created with the versions of all the solvers it
was created with.
I'm actually more in favor of including the SMT version in the file (and
as part of the input to the hash for the file's name). I don't see it as a
lot of additional work, and while the primary utilization of the cache is
intended to be the CI, the caching is being implemented by saw-script
itself and not the CI, so I think it should be robust against other uses.
It's also worth noting that the what4 repo tests multiple different solver
versions during its CI and that might be desirable for saw at some future
point as well.
—
Reply to this email directly, view it on GitHub
<#1873 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABYQSOTGC5ST3IJQWPZHLPDXHAPSDANCNFSM6AAAAAAYICQ3C4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
I would suggest to use a persistent key/value store rather than a SQL database. I think LMDB is simple to use and fits well. There are haskell binding, such as lmdb-simple. Alternatively, RocksDB (https://rocksdb.org/) supports more features, but I don't think we require any here. There are haskell binding as well, such as rocksdb-haskell. |
For the record, both of these things are done currently. Once I make sure the CI passes on my latest commit, I'm also going to add a
There's no easy way to save the exact command line options passed to each solver, since each backend (i.e. What4, SBV, etc.) has its own system for actually calling out to solvers. However, as part of version information we save which backend was used as well as all additional options the user provides. For example, results obtained with
Wow, this is perfect – thanks for the suggestion! The one thing I wasn't sure about was how to properly handle the fact that we now have a new C dependency for |
I'm late with this feedback, but I think |
For some reason I got the impression from reading the docs around That being said, I don't have very much time left to work on this task, and something like this doesn't seem super easy to configure and get working (especially on CI). If Ryan comes back and says it's super easy then I can try to get it working, otherwise someone else might have to jump in. But, if someone just gets |
Sorry, I'm missing context on the linking issues you're alluding to. Can you describe what the problem is in more detail? |
@RyanGlScott Originally, this PR used the |
I still feel like I'm missing something. After all, the approach you are using still requires users to install LMDB themselves, but via Python instead of Haskell. I'm not sure why one approach is considered too much while the other isn't... |
@RyanGlScott Sorry, maybe this will be more helpful. The difference is:
|
OK. If I understand correctly, the issue isn't really in static vs. dynamic linking, but rather requiring users to install a third-party C library that powers an optional feature in SAW. From this perspective, I don't see that great of a difference between options (1) and (3), as statically linking against Unfortunately, this sort of thing is awkward to accomplish. I can foresee two ways to make this not terrible for SAW users, but both of them would require downstream changes to the Haskell
Of these options, (1) would probably be easiest, considering that the Python bindings already do this. |
Ah, okay – thanks, Ryan! For some reason I thought static linking would work similar to (1), i.e. that the C parts would be compiled into the Given the amount of time I have left to work on this, I think I'd prefer to just keep the current Python solution instead of trying to write a new Haskell LMDB library. @andreistefanescu what do you think? On another note: I think I've addressed all of your comments now, @RyanGlScott! The only things left that I think need review are the new docs I added in 0e2efb2 and bbdfdf0, as well as the question I asked about running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I thought static linking would work similar to (1), i.e. that the C parts would be compiled into the
saw
executable
To be clear, that is how static linking works. The contentious bit is where the static library comes from, i.e., whether the user has to install it themselves, or if they can use a copy of the library that is bundled with lmdb
itself.
In any case, I'm fine with using the Python approach. If you want, you could file an issue to track the idea of switching to an in-Haskell solution.
Okay, this is ready for review again! Here's what's been done:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some new test failures in the integration tests CI job. I've taken another pass through SAWScript.SolverCache
now that it is based on lmdb-simple
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, I'm quite happy with how this has turned out. Great work, @m-yac!
This PR adds a solver-backend-aware system for caching SMT solver results.
SolverCache.hs
for more information on how the system works internally.Example (Out of date, see the manual for an update)
First we enable caching to disk and run a few queries:
Next, we can use
print_solver_cache
to see what was cached (print_solver_cache ""
will show all entries) andprint_solver_cache_stats
to get statistics about how the solver cache has been used so far this run:Finally, we can start up
saw
again and verify that these results will be used – in particular, order doesn't matter, local variable names don't matter, but the solver backend does:There are also two other ways to interact with solver caching:
enable_solver_cache
command, which turns on solver result caching in memory but does not save anything to disk. Ifset_solver_cache_path
is later run, everything from memory is written to disk. Ifset_solver_cache_path
is never run, the cached results are forgotten aftersaw
execution ends.SAW_SOLVER_CACHE_PATH
environment variable, which has the same behavior as callingset_solver_cache_path
with the value of the variable at the start of execution.A quick and dirty test on a larger example shows that there is a performance improvement – in the case of
intTests/test_w4/prims.saw
on my machine (see below) it takes 3.467s to run the test with caching turned off, 3.541s to run the test with caching turned on but nothing in the cache (so 0.1s of extra time spent caching the entries), and 2.540s to run the test with a loaded cache (so 1s less time calling solvers, a ~30% speed increase). I imagine for files with more complicated SMT queries thanprims.saw
(which mostly contains zeroth-order queries such as(16:[8]) >> 3 == 2
), this speed increase can get much larger.Additional Changes
Note that this PR also makes the following changes:
Setup.hs
script now also includes AIG, What4, and SBV version information inGitRev.hs
, used byBuiltins.hs
andInterpreter.hs
to generate solver backend version information for caching. Accordingly thesaw-script
library and executable now also depend onGitRev
insaw-script.cabal
.sequentToSATQuery
on its givenSequent
, we now callsequentToSATQuery
in one place (applyProverToGoal
) and pass the resultingSATQuery
to each backend directly.