Bounded interpreter cache support#316
Conversation
Is it reseted using the already existing |
Yes. |
This will allow us to free FlatMap memory and allocate it lazily when needed. Therefore, optimizing run time memory usage. Signed-off-by: Aman <aman@parity.io>
...and automatic cache reset when quota is exceeded. Instead of adding a check in the instruction execution loop, we cleverly replace the first instruction of the new basic block with reset cache micro instruction. This keeps the runner loop optimized. In addition, we make sure capacity of the interpreter cache never gets too big. Signed-off-by: Aman <aman@parity.io>
|
Latest commit adds support for #309 |
athei
left a comment
There was a problem hiding this comment.
AFAIK this is also missing the memory consumed by the flatmap.
koute
left a comment
There was a problem hiding this comment.
Took a brief look; I'll finish reviewing later.
|
Thanks @koute, I've addressed your comments in the latest revision. |
athei
left a comment
There was a problem hiding this comment.
API looks good to me now. But lets wait to Jan's approval.
|
Updated my PR to make use of your calculation: paritytech/polkadot-sdk#9267 Seems to work as expected. |
|
@athei Is your PR the final version? You're using |
Oops yeah. I forgot to set the limit. I am just resetting the cache on every sub call. Will add the limit. |
|
Updated: paritytech/polkadot-sdk@032e005 |
|
Okay, so the number of corner cases that I had to point out in the review makes my head hurt a little, which is a sign that we should let the machine figure out whether this is correct or not. :P So, I don't want to delay this PR any longer, but once we merge this let's do #320 |
| assert_eq!(instance.reg(Reg::A2), 0x4567); | ||
| } | ||
|
|
||
| fn bounded_interpreter_cache(config: Config) { |
There was a problem hiding this comment.
This test can be slightly improved I think.
- Change every basic block to do something different, otherwise executing from the first block and from the fourth block is indistinguishable (every block should have a different effect); e.g. you could just have every basic block set a different single register.
- Make the jumps non-monotonic (i.e. have them jump to blocks which are not the next block; as-is those jumps could have as well be replaced with a
fallthrough) - Call into the program multiple times (using the same instance), into randomly selected basic blocks (where "random" here is just some arbitrary order you pick.)
- Add an assert that
max_cache_size_bytesreturns an error when some small value is passed to it, e.g.1or something like that. - Add an assert that
max_cache_size_bytesdoesn't return an error when some huge value is passed to it, e.g.100000or something like that. - Test with the minimum possible
max_cache_size_bytes(easiest way to get minimum without hardcoding: just increment it untilset_interpreter_cache_size_limitdoesn't return an error)
There was a problem hiding this comment.
Done. Thanks for the recommendations!
...and also return memory info, such as baseline memory usage and purgeable memory size. In addition, we are making max_cache_size_bytes an upper bound on the cache size. Signed-off-by: Aman <aman@parity.io>
Add support for bounded interpreter cache and automatic cache reset when quota is exceeded.
Instead of adding a check in the instruction execution loop, we cleverly replace the first instruction of the new basic block with reset cache micro-instruction. This keeps the runner loop optimized. In addition, we make sure capacity of the interpreter cache never gets too big.
As you may notice,
max_cache_sizeis not a tight upper bound, and transiently we could allocate more thanmax_cache_size. This is needed to keep the implementation optimized and complexity at bay.Finally, we are also adding support to reset FlatMap and allocate its memory on first use.
Testing
Fixes: #315