Correctly purge cache and refactor State::ensureCached #3378
Conversation
| auto it = m_cache.find(_id); | ||
| if (asserts(it != m_cache.end())) | ||
| Account* a = account(_id); | ||
| if (asserts(a != nullptr)) |
There was a problem hiding this comment.
I agree, but this is just a refactoring.
| void State::kill(Address _a) | ||
| void State::ensureAccountExists(const Address& _address) | ||
| { | ||
| if (!addressInUse(_address)) |
There was a problem hiding this comment.
Please do not use another helper function here.
There was a problem hiding this comment.
Do you want to remove addressInUse completely?
There was a problem hiding this comment.
It is confusing for me that we have: "exists", "is empty", and "in use" queries. My guess is "in use" should go away :).
| return NullBytes; | ||
| ensureCached(_contract, true, false); | ||
| return m_cache[_contract].code(); | ||
| return account(_a, true)->code(); |
There was a problem hiding this comment.
This is bad design because you have to search twice. Maybe the Account should have a method "fetch code".
There was a problem hiding this comment.
The account does not have access to the database and I think we should leave it like that for now, so the account cannot fetch the code.
I think we should really focus on readability, encapsulation and correctness here. If we had had modularity in this file, the bug you just fixed would not have been introduced.
Furthermore, the cost we have to pay here is a potential (compiler might optimize) search in a memory cache, compared to the worst case cost of disk access.
| return size; | ||
| if (a->isFreshCode()) | ||
| return code(_a).size(); | ||
| auto& codeSizeCache = CodeSizeCache::instance(); |
There was a problem hiding this comment.
Now I can see that this global code size cache is bad idea. We should have a good account cache (as we need it anyway) and keep the code size information in the account itself.
There was a problem hiding this comment.
Can you please elaborate? It has to be global in order to speed up code size lookup for two contracts that have the same code - that is its only purpose. In addition, we can add another cache inside the account, but I'm not sure if that is an improvement because the account data structure will grow.
There was a problem hiding this comment.
We have the account cache and the code size cache. But the account is a tuple (nonce, balance, ptr to code, ptr to storage). It can be easily extended to tuple (nonce, balance, code size, ptr to code, ptr to storage) and the we can drop the code size cache. Just the idea, not request for changes in this PR.
| /// Retrieve certain information about a given address into the cache and return | ||
| /// a pointer to the relevant account (or a null pointer if it does not exist). | ||
| /// If _requireCode is true, also load the code. | ||
| Account* ensureCached(Address const& _a, bool _requireCode) const; |
There was a problem hiding this comment.
You are too pedantic about const here. Let's keep the single function instead of 3 of them.
There was a problem hiding this comment.
I would prefer a solution with only two functions, but I think it is important to distinguish between const and non-const access to the accounts. This might spare us another bug.
There was a problem hiding this comment.
How about using const_cast and having 2 function instead of 3?
| return size; | ||
| if (a->isFreshCode()) | ||
| return code(_a).size(); | ||
| auto& codeSizeCache = CodeSizeCache::instance(); |
There was a problem hiding this comment.
We have the account cache and the code size cache. But the account is a tuple (nonce, balance, ptr to code, ptr to storage). It can be easily extended to tuple (nonce, balance, code size, ptr to code, ptr to storage) and the we can drop the code size cache. Just the idea, not request for changes in this PR.
| /// Retrieve certain information about a given address into the cache and return | ||
| /// a pointer to the relevant account (or a null pointer if it does not exist). | ||
| /// If _requireCode is true, also load the code. | ||
| Account* ensureCached(Address const& _a, bool _requireCode) const; |
There was a problem hiding this comment.
How about using const_cast and having 2 function instead of 3?
| void State::kill(Address _a) | ||
| void State::ensureAccountExists(const Address& _address) | ||
| { | ||
| if (!addressInUse(_address)) |
There was a problem hiding this comment.
It is confusing for me that we have: "exists", "is empty", and "in use" queries. My guess is "in use" should go away :).
When removing random accounts from cache, remove it also from "unchanged accounts" set.
No description provided.