-
Notifications
You must be signed in to change notification settings - Fork 57
Optimize memory representation and operations #707
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -160,6 +160,7 @@ makeVm o = do | |
, iterations = mempty | ||
, config = RuntimeConfig | ||
{ allowFFI = o.allowFFI | ||
, minMemoryChunk = o.minMemoryChunk | ||
, baseState = o.baseState | ||
} | ||
, forks = Seq.singleton (ForkState env block cache "") | ||
|
@@ -674,9 +675,9 @@ exec1 conf = do | |
mcopy sz srcOff dstOff = do | ||
m <- gets (.state.memory) | ||
case m of | ||
ConcreteMemory mem -> do | ||
buf <- freezeMemory mem | ||
copyBytesToMemory buf sz srcOff dstOff | ||
ConcreteMemory _ -> do | ||
buf <- readMemory srcOff sz | ||
copyBytesToMemory buf sz (Lit 0) dstOff | ||
SymbolicMemory mem -> do | ||
assign (#state % #memory) (SymbolicMemory $ copySlice srcOff dstOff sz mem mem) | ||
|
||
|
@@ -2953,7 +2954,12 @@ writeMemory memory offset buf = do | |
expandMemory targetSize = do | ||
let toAlloc = targetSize - VUnboxed.Mutable.length memory | ||
if toAlloc > 0 then do | ||
memory' <- VUnboxed.Mutable.grow memory toAlloc | ||
vm <- get | ||
-- If you are using pure concrete mode, use a large chunk (e.g. 64k). | ||
-- We want to always grow at least a chunk, to avoid the performance impact | ||
-- that would happen with repeated small expansion operations, as grow does | ||
-- a larger *copy* of the vector on a new place | ||
memory' <- VUnboxed.Mutable.grow memory $ max toAlloc vm.config.minMemoryChunk | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe this should be a growth factor rather than a fixed min amount There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice find! So I had lots of issues in C++ where I was growing by a fixed chunk... (side-note: never use |
||
assign (#state % #memory) (ConcreteMemory memory') | ||
pure memory' | ||
else | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,6 +44,7 @@ vmForEthrunCreation creationCode = | |
, allowFFI = False | ||
, freshAddresses = 0 | ||
, beaconRoot = 0 | ||
, minMemoryChunk = 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this controllable? We can just set it to a sane default instead? I'd rather not have config options for stuff that a good default works for. It's useful to have this during figuring out what a good default is, but later, it may become a distraction perhaps? What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern that led me to make this a tunable was symbolic execution. I'm not particularly familiar with how it is represented though so take this with a grain of salt, but what would happen if you end up having a large concrete memory due to the "optimistic growth", which then turns symbolic due to some other reason? How would that get encoded into Z3 et al? Would it hurt performance? If my concern is unfounded I'm all for picking a good default and not having to have this option 😄 |
||
}) <&> set (#env % #contracts % at (LitAddr ethrunAddress)) | ||
(Just (initialContract (RuntimeCode (ConcreteRuntimeCode "")))) | ||
|
||
|
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.
I think this previous code results in a bit simpler representation for the symbolic case, so we might want to keep it and only modify the concrete case