OC tierup compile in order #1342
Conversation
…der. Prioritize eosio.*.
| @@ -80,7 +80,7 @@ class code_cache_base { | |||
|
|
|||
| //these are really only useful to the async code cache, but keep them here so | |||
| //free_code can be shared | |||
| std::unordered_set<code_tuple> _queued_compiles; | |||
| deque<code_tuple> _queued_compiles; | |||
There was a problem hiding this comment.
I'm worried about the performance ramification of switching this from constant logn time look up to linear time lookup. The lookup occurs every time an action is executed and when someone starts fresh there could be hundreds or thousands of entries that need to be searched through for every action (until compilation settles down).
I think maybe instead we should have a second
std::unordered_set<code_tuple> _high_priority_queued_compiles;There was a problem hiding this comment.
One issue I saw was that compiling was according to code_hash sort order instead of order they came in. This preserves the order. If worried about performance, maybe we should use a multindex instead for this which would allow for keeping track of order, last used, prioritize eosio.*, and provide fast lookup.
There was a problem hiding this comment.
yeah if you want to maintain compilation order, a multi_index sounds good
| @@ -156,13 +157,17 @@ const code_descriptor* const code_cache_async::get_descriptor_for_code(const dig | |||
| it->second = false; | |||
| return nullptr; | |||
| } | |||
| if(_queued_compiles.find(ct) != _queued_compiles.end()) { | |||
| if(auto it = _queued_compiles.get<by_hash>().find(boost::make_tuple(std::ref(code_id), vm_version)); it != _queued_compiles.get<by_hash>().end()) { | |||
| _queued_compiles.relocate(_queued_compiles.begin(), _queued_compiles.project<0>(it)); | |||
There was a problem hiding this comment.
Doesn't this eliminate the strict prioritization of system contracts? imo that's a more important behavior to uphold than trying to prioritize based on MRU. Maybe just remove this relocate() which will then make it FIFO, with the exception that system contracts get push_front()ed to the front of the line.
Being FIFO is still better than it was previously.
| @@ -106,7 +107,7 @@ std::tuple<size_t, size_t> code_cache_async::consume_compile_thread_queue() { | |||
| } | |||
|
|
|||
|
|
|||
| const code_descriptor* const code_cache_async::get_descriptor_for_code(const digest_type& code_id, const uint8_t& vm_version, bool is_write_window, get_cd_failure& failure) { | |||
| const code_descriptor* const code_cache_async::get_descriptor_for_code(const account_name& receiver, const digest_type& code_id, const uint8_t& vm_version, bool is_write_window, get_cd_failure& failure) { | |||
There was a problem hiding this comment.
I think it's fine to leave as is, but for increased abstraction maybe this just takes bool high_priority instead, and the receiver.prefix() == chain::config::system_account_name check is then left outside of any eos-vm-oc code
There was a problem hiding this comment.
I like that. Didn't feel right including chain::config here.
Compile contracts in order executed except for
eosio.*contracts. Prioritizeeosio.*accounts to be executed next.