Use Map-based data structure for LLVM allocation histories. #550
Conversation
|
|
||
| instance Semigroup (MemAllocs sym) where | ||
| (MemAllocs lhs_allocs) <> (MemAllocs rhs_allocs) | ||
| | Just (lhs_head_allocs, Allocations lhs_m) <- List.unsnoc lhs_allocs |
There was a problem hiding this comment.
If this semigroup operation is called a lot, it may be worth optimizing access to the tail of the list by using some double-sided data structure, like Data.Sequence.
There was a problem hiding this comment.
In practice, I believe this semigroup operation is really only used to cons new operations onto the front, so a list probably isn't too bad. Changing the datatype would also require changing all the functions that traverse MemAllocs, which currently use pattern matching on the list constructors. On the other hand, making that traversal code more abstract wouldn't be a bad thing.
In either case, if we switch MemAllocs to use Data.Sequence, we should probably do MemWrites as well.
There was a problem hiding this comment.
I'd be in favor, generally, of making those traversals and such more abstract to provide us with more flexibility in the future with representation choices. I don't know if it should be part of this PR or not.
There was a problem hiding this comment.
Revision 151e1f9 makes MemAllocs into an abstract datatype, so now all the traversals are done using the abstract interface provided by Lang.Crucible.LLVM.MemModel.MemLog.
|
I could use some advice/suggestions for how to make some proper tests for this code. |
|
It looks like the macos CI builds are failing in the same way that the saw-script builds were doing recently: I think @jared-w managed to fix the problem for saw-script by changing some cache settings; we probably need to do the same for the crucible repo. |
|
Unfortunately the only reliable thing I've been able to do so far is just remove the cache by changing the cache key in the github actions file and seeing if that does anything. I've gotten the cache as narrow as I can get it at this point. One thing I have next on my list to try is to use cabal-cache which has more advanced caching logic than just grabbing all of |
brianhuffman
force-pushed
the
MemAllocs
branch
from
fd5d97f
to
151e1f9
Compare
|
Regarding tests, take a look at crucible/crucible-llvm/test/Tests.hs Line 479 in b8a466a doAlloc and/or the allocation API.
|
|
I agree that adding tests for this is worthwhile, but I don't think we should let this PR linger unmerged. |
Instead of recording allocations sequentially, we now store collections of allocations together in a map indexed by block ID. This makes lookups much faster in the common case where the block ID we are checking is concrete. Fixes #549.
All operations that traverse a MemAllocs log are now implemented within the Lang.Crucible.LLVM.MemModel.MemLog module.
Internal invariants of this abstract datatype are moved to an ordinary comment because they are not relevant to the exported API.
brianhuffman
force-pushed
the
MemAllocs
branch
from
dd4cb31
to
9a304ce
Compare
|
9a304ce adds a few simple tests alongside |
Instead of recording allocations sequentially, we now store
collections of allocations together in a map indexed by block
ID. This makes lookups much faster in the common case where
the block ID we are checking is concrete.
Fixes #549.