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

43 observable sharing #76

Merged
merged 19 commits into from
Aug 21, 2024
Merged

43 observable sharing #76

merged 19 commits into from
Aug 21, 2024

Conversation

bruderj15
Copy link
Owner

No description provided.

@bruderj15 bruderj15 linked an issue Aug 16, 2024 that may be closed by this pull request
@bruderj15 bruderj15 marked this pull request as ready for review August 17, 2024 13:28
@fabeulous
Copy link

It's nice to see the sharing worked out for SMT and OMT 👍

Unfortunately, the sharing in Pipe doesn't interact well with MonadIncrSMT.

For example:

example :: IO ()
example = do
  z3Inter <- interactiveSolver z3
  res <- debugInteractiveWith @Pipe (z3Inter) $ do
     setLogic "QF_LIA"
     x <- var @IntSort
     let a1 = x + x
         an = a1 === 2
     push
     assert an
     pop
     assert an
     solve
     return ()
  return ()

will produce a runtime error

"(error \"line 1 column 199: unknown constant var_3\")"
lpo-encoding: string
CallStack (from HasCallStack):
  error, called at src/Language/Hasmtlib/Type/Pipe.hs:114:9 in hasmtlib-2.3.2-inplace:Language.Hasmtlib.Type.Pipe

This is down to sharing the expression of an, but only introducing the corresponding variable var_2 after the push. The pop removes the variable and assertion from the stack, but var_2 is reused in the second assert.

(set-logic QF_LIA)
(declare-fun var_1 () Int)
(push 1)
(declare-fun var_2 () Int)
(assert (= var_2 (+ var_1 var_1)))
(declare-fun var_3 () Bool)
(assert (= var_3 (= var_2 2)))
(assert var_3)
(pop 1)
(assert var_3)
(check-sat)
(error "line 1 column 199: unknown constant var_3")

@bruderj15
Copy link
Owner Author

Good catch!
Most simple solution seems to be clearing the entire stableMap on a pop. However we may loose out on a lot of possible sharing doing so.
Better: Similar to the Solver, keep track of the assertions on the incremental stack. Then delete corresponding entries in stableMap when popping.
I will add this to the PR.

@bruderj15
Copy link
Owner Author

That solution is a little ugly but keeps memory overhead little.
Your example now results in:

(set-logic QF_LIA)
(declare-fun var_1 () Int)
(push 1)
(declare-fun var_2 () Int)
(assert (= var_2 (+ var_1 var_1)))
(declare-fun var_3 () Bool)
(assert (= var_3 (= var_2 2)))
(assert var_3)
(pop 1)
(declare-fun var_4 () Int)
(assert (= var_4 (+ var_1 var_1)))
(declare-fun var_5 () Bool)
(assert (= var_5 (= var_4 2)))
(assert var_5)

Copy link

@fabeulous fabeulous left a comment

Choose a reason for hiding this comment

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

Nice! The added stack fixes the issue I mentioned 👍

While testing I only noticed two possible issues with performance (see my other comments).


pop = do
smt <- get
let cmd = "(pop 1)"
when (smt^.isDebugging) $ liftIO $ ByteString.Char8.putStrLn $ toLazyByteString cmd
liftIO $ B.command_ (smt^.pipe) cmd
{-# INLINE pop #-}
forMOf_ (stackHeightAuxMap.ix (smt^.incrStackHeight).folded) smt (\sn -> pipeStableMap.at sn .= Nothing)

Choose a reason for hiding this comment

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

Here you likely want to clean up the stack with something like:

stackHeightAuxMap.at (smt^.incrStackHeight) .= Nothing

otherwise, the stack level will not be empty after another push call.
For me adding this reduces the runtime of

example :: IO ()
example = do
  z3Inter <- interactiveSolver z3
  debugInteractiveWith @Pipe (z3Inter) $ do
    setLogic "QF_LIA"
    x <- var @IntSort
    forM_ [1..5000] $ \_ -> do
      let bn = nest 2 x
          an = bn === 2
      push
      assert an
      pop
  return ()
 where 
  nest 0 x = x
  nest n x = nest (n-1) (x + x)

from 7.3s to 0.33s.

Since you only access the stackHeightAuxMap at incrStackHeight, a simpler stack representation like [[StableName ()]] could also be used.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Great input once again.
We will clean the stack and replace the IntMap with Seq (Seq (StableName ())).

return nodeVar

makeStableName' :: GNFData f => f a -> IO (StableName ())
makeStableName' x = grnf x `seq` fmap unsafeCoerce (makeStableName x)

Choose a reason for hiding this comment

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

I think grnf reducing the expression to normal form is unnecessary. For makeStableName it should be enough to evaluate to weak head normal form with:

makeStableName' x = x `seq` fmap unsafeCoerce (makeStableName x)

For deeply nested expressions with a lot of sharing grnf could have exponential run-time compared to just seq. For the following example removing grnf reduces the runtime from 29s to 0.01s on my machine, while producing the same SMTLib output.

example :: IO ()
example = do
  z3Inter <- interactiveSolver z3
  debugInteractiveWith @Pipe (z3Inter) $ do
    setLogic "QF_LIA"
    x <- var @IntSort
    let bn = nest 30 x
        an = bn === 2
    push
    assert an
    pop
  return ()
 where 
  nest 0 x = x
  nest n x = nest (n-1) (x + x)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Right, overshot here.

@bruderj15
Copy link
Owner Author

I highly appreciate your help, many thanks!

@bruderj15 bruderj15 merged commit c234cf4 into develop Aug 21, 2024
6 checks passed
@bruderj15 bruderj15 deleted the 43-observable-sharing branch August 21, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Observable sharing
2 participants