Add eos-vm-oc-enable auto mode by heifner · Pull Request #1322 · AntelopeIO/leap

heifner · 2023-06-20T19:51:33Z

Short Description

In order to enhance the performance of EOS and EOS EVM, we are introducing the use of OC (Optimizing Compiler) for the EOS VM. This initiative aims to improve the overall efficiency and scalability of the platform.

What

The primary goal of this initiative is to leverage OC for the EOS VM by implementing an Auto OC Mode as the default setting. This new mode, eos-vm-oc-enable=auto, will enhance performance. OC will automatically be used based on different contexts, such as building blocks, applying blocks, executing transactions from HTTP or P2P, and various transaction types. OC will also be used for all contracts on eosio.* accounts. This includes: eosio, eosio.token, eosio.ibc, and eosio.evm.

Why

By utilizing OC, we can significantly boost the performance of EOS and EOS EVM. With the Auto OC Mode as the default, the system will automatically apply OC when appropriate, optimizing the execution of transactions and improving overall throughput. The introduction of OC for specific contexts, such as building blocks and speculative transaction executions, further enhances the efficiency of the platform.

Auto OC Mode as default

eos-vm-oc-enable=auto is the new default. Scripts that use --eos-vm-oc-enable will need to be updated to use --eos-vm-oc-enable all to enable OC for all contexts.

eos-vm-oc-enable=true => eos-vm-oc-enable=all (eos-vm-oc-enable=true continues to work and is mapped to all)
eos-vm-oc-enable=false => eos-vm-oc-enable=none (eos-vm-oc-enable=false continues to work and is mapped to none)
--eos-vm-oc-enable will not now work, replace with --eos-vm-oc-enable all

Baseline / OC selection

In Auto OC Mode, leverage EOS VM OC, as follows:

Context	OC?
Building block	baseline, but OC if account is eosio.*
Applying block	OC, or if producer: baseline, but OC if account is eosio.*
Speculatively executing trx from HTTP	baseline, but OC if account is eosio.*
Speculatively executing trx from p2p	baseline, but OC if account is eosio.* 💡
Compute transaction	baseline, but OC if account is eosio.*
Read only transaction	OC

💡 Although enabling OC for P2P would enable quicker validation of speculative trxs as they traverse the network, this PR does not add OC for P2P to help prevent overload on BP nodes. A separate option to enable OC for P2P when in auto is being considered as a follow on enhancement; see #1333.

Resolves #1251

…e=auto

heifner · 2023-06-20T19:52:54Z

libraries/chain/include/eosio/chain/wasm_interface_collection.hpp

+   /**
+    * @class wasm_interface_collection manages the active wasm_interface to use for execution.
+    */
+   class wasm_interface_collection {


There is no functionality change here, just a refactor out of controller.

heifner · 2023-06-20T19:55:21Z

libraries/chain/include/eosio/chain/controller.hpp

      void replace_account_keys( name account, name permission, const public_key_type& key );

+      void set_producer_node(bool is_producer_node);
+      bool is_producer_node()const;


Kind of sad about this change. Until now, the core of leap had no need to know if it was a producer or not.

well, I guess depending how much of a purist you want to be here, maybe you could make controller take a std::function<bool(apply_context)> tier_up_with_oc, either as a ctor parameter or a config or something. That way chain_plugin can decide how to set this up based on the various configs. And libtester just always passes in a return false; for example, I guess.

Yeah, that seems a bit too far.

heifner · 2023-06-20T19:55:40Z

libraries/chain/include/eosio/chain/name.hpp

+       *    "eosio.any" -> "eosio"
+       *    "eosio" -> "eosio"
+       */
+      constexpr name prefix() const {


Implementation copied from CDT

spoonincode · 2023-06-21T15:41:27Z

plugins/chain_plugin/chain_plugin.cpp

+     v = boost::any(wasm_interface::vm_oc_enable::oc_auto);
+  } else if (s == "all" || s == "true" || s == "on") {
+     v = boost::any(wasm_interface::vm_oc_enable::oc_all);
+  } else if (s == "none" || s == "false" || s == "off") {


Other previously valid choices were 1/0 and yes/no

…ime to match genesis used in other tests. This allows for setcode in slow ci/cd env.

docs/01_nodeos/03_plugins/chain_plugin/index.md

libraries/chain/apply_context.cpp

libraries/chain/controller.cpp

libraries/chain/include/eosio/chain/name.hpp

unittests/misc_tests.cpp

linh2931 · 2023-06-22T15:00:41Z

plugins/chain_plugin/chain_plugin.cpp

+     throw validation_error(validation_error::invalid_option_value);
+  }
+}
+


To have fewer options in the future, we could add a deprecation note that only auto, all, and none will be allowed, all others could be deprecated in future releases.

linh2931 · 2023-06-22T15:03:03Z

libraries/chain/include/eosio/chain/wasm_interface_collection.hpp

+         std::unordered_map<std::thread::id, std::unique_ptr<wasm_interface>> threaded_wasmifs; // one for each read-only thread, used by eos-vm and eos-vm-jit
+   };
+
+} // eosio::chain


Thanks for this refactoring. It hides those details from the controller.

Can we add some tests for auto, all, and none options to make sure OC is enabled/disabled as configured?

Any ideas on how? Unfortunately, oc is not setup to be used in unittests because the destructor of tester -> controller destroys oc and it can't be restarted because it forks the process on start. Seems like it would require a major refactor to allow oc to be used repeatably in unittests.

We could add some logging and look for a particular log in some integration tests. Let me see what I can do for that.

That's surprising to hear -- it was definitely originally designed so that multiple controllers using OC can exist simultaneously: we have unittests that do that.

But, that was for the non-tier-up usage of OC. So maybe there was a shortcoming of the original impl in this area. Or maybe we've regressed some how.

Integration test added.

…me to be found in logs on slow ci/cd machine

spoonincode · 2023-06-28T02:13:06Z

libraries/chain/include/eosio/chain/wasm_interface_collection.hpp

+            EOS_ASSERT(!is_on_main_thread(), misc_exception, "init_thread_local_data called on the main thread");
+            if (is_eos_vm_oc_enabled()) {
+               // EOSVMOC needs further initialization of its thread local data
+               wasmif.init_thread_local_data();


lost an #ifdef EOSIO_EOS_VM_OC_RUNTIME_ENABLED on these couple lines during the refactor

I wonder if most of the #ifdef EOSIO_EOS_VM_OC_RUNTIME_ENABLED could be removed to simplify differences. For now, I'll just revert back to having them.

Restored #ifdefs

spoonincode · 2023-06-28T02:32:19Z

libraries/chain/include/eosio/chain/wasm_interface_private.hpp

      }

+      bool should_always_oc_tierup()const {
+         return wasm_runtime_time == wasm_interface::vm_type::eos_vm_oc || eosvmoc_tierup == wasm_interface::vm_oc_enable::oc_all;


I don't think wasm_runtime_time == wasm_interface::vm_type::eos_vm_oc should be in here. wasm_runtime_time == wasm_interface::vm_type::eos_vm_oc is meant to be "OC in non-tier up mode" and all the code back here

leap/libraries/chain/wasm_interface.cpp

Line 93 in 6008c72

if(my->eosvmoc && (my->should_always_oc_tierup() || context.should_use_eos_vm_oc())) {

should be skipped entirely in non-tier up mode. It's not clear what the ramifications are for having both a synchronous and asynchronous code_cache at the same time (which is what I think might be happening in this case.. not sure.. regardless it def wasn't originally intended to operate this way)

I think the line above should be something more on the order of

if(my->eosvmoc && (my->eosvmoc_tierup == wasm_interface::vm_oc_enable::oc_all || context.should_use_eos_vm_oc())) {

Thanks. I missed that eos_vm_oc didn't flow throw here.

It seems a non issue from a functional standpoint -- I remember now that my->eosvmoc isn't created unless in oc_auto or oc_all mode,

leap/libraries/chain/include/eosio/chain/wasm_interface_private.hpp

Lines 93 to 96 in a8f20ce

if(eosvmoc_tierup != wasm_interface::vm_oc_enable::oc_none) {

EOS_ASSERT(vm != wasm_interface::vm_type::eos_vm_oc, wasm_exception, "You can't use EOS VM OC as the base runtime when tier up is activated");

eosvmoc.emplace(data_dir, eosvmoc_config, d);

}

so nothing wonky would have happened since my->eosvmoc && would have always failed out. But yeah def better now with the tweak.

spoonincode · 2023-06-28T02:35:40Z

libraries/testing/include/eosio/testing/tester.hpp


            for(int i = 0; i < boost::unit_test::framework::master_test_suite().argc; ++i) {
+               // don't use auto tier up for tests, since the point is to test diff vms
+               cfg.eosvmoc_tierup = chain::wasm_interface::vm_oc_enable::oc_none;


Not sure why this is inside the for() loop instead of just with the cfg. items a few lines above.

spoonincode · 2023-06-28T02:39:27Z

plugins/chain_plugin/chain_plugin.cpp

+     v = boost::any(wasm_interface::vm_oc_enable::oc_auto);
+  } else if (s == "all" || s == "true" || s == "on" || s == "yes" || s == "1") {
+     v = boost::any(wasm_interface::vm_oc_enable::oc_all);
+  } else if (s == "none" || s == "false" || s == "off" || s == "no" || s == "0") {


fwiw, it appears boost was doing a case insensitive compare; so False, oN, etc was also accepted 😈 depending on how accommodating you want to be...

spoonincode · 2023-06-28T03:02:57Z

plugins/chain_plugin/chain_plugin.cpp

-         ("eos-vm-oc-enable", bpo::bool_switch(), "Enable EOS VM OC tier-up runtime")
+         ("eos-vm-oc-enable", bpo::value<chain::wasm_interface::vm_oc_enable>()->default_value(chain::wasm_interface::vm_oc_enable::oc_auto),
+          "Enable EOS VM OC tier-up runtime ('auto', 'all', 'none').\n"
+          "'auto' - EOS VM OC tier-up is enabled for eosio.* accounts and read-only trxs.\n"


I know Lin already touched on it some, but I do feel like we ought to try and find a short concise way to sneak in that it's also enabled for applying blocks unless a BP. That aspect is completely missing in this description even without the extra "non BP rule".

Maybe something like

'auto' - EOS VM OC tier-up is enabled for eosio.* accounts, read-only trxs, and applying blocks (excluding on producers).

We already have plenty more wordy config items in read-mode, peer-log-format, etc.. so I feel like we can spend 5 words.

whoops somehow I was on an older revision when I made that comment. I see now that we did add commentary around applying blocks. Still not sure penny pinching 3 words and this avoiding documenting the producer exclusion is worth it... but what we have in the latest commit is fine to me since at least it includes block application

What do you think of:
'auto' - EOS VM OC tier-up is enabled for eosio.* accounts, read-only trxs, and except on producers applying blocks.. Putting it as a paratheses at the end made it seem like producer excluded all cases.

spoonincode · 2023-06-28T03:05:21Z

plugins/state_history_plugin/tests/main.cpp

@@ -0,0 +1,2 @@
+#define BOOST_TEST_MODULE state_history_plugin
+#include <boost/test/included/unit_test.hpp>


Since you're using the header-only variant now, you should remove linking to the library from here

leap/plugins/state_history_plugin/tests/CMakeLists.txt

Line 2 in 8706738

target_link_libraries(test_state_history state_history_plugin eosio_testing eosio_chain_wrap Boost::unit_test_framework)

…actor

spoonincode

looks good

spoonincode · 2023-06-29T02:41:33Z

libraries/chain/wasm_interface.cpp

         }
         if(cd) {
+            if (!context.is_applying_block())
+               tlog("${a} speculatively executing ${h} with eos vm oc", ("a", context.get_receiver())("h", code_hash));


was this meant to be left in?

tlog is trace log, but no, that was for testing.

Sorry, yes, it is needed for the read_only_trx_test.py test. I'll add a comment about that.

spoonincode · 2023-06-29T02:41:36Z

libraries/chain/webassembly/runtimes/eos-vm-oc/code_cache.cpp

 }

 void code_cache_base::set_on_disk_region_dirty(bool dirty) {
+   // tests can remove directory before destructor is called


This feels pretty weird to me.. it makes it sound like there are cases where fc::temp_dir is dtor'ed before the controller is dtor'ed?

Good point. I'll fix the tests and remove this.

…s it

heifner added 10 commits June 16, 2023 16:29

GH-1251 Use enum for eosvmoc_tierup instead of bool.

9229b34

GH-1251 Refactor wasm_interface map into a wasm_interface_collection

a05077a

GH-1251 Fix failing tests

c167373

GH-1251 Fix tests to be able to run with default of --eos-vm-oc-enabl…

e7f1406

…e=auto

GH-1251 Add prefix() method to name

34fa470

GH-1251 Fix test

360ffb5

GH-1251 Remove unused

23f6db5

GH-1251 Update docs

1cc64a0

GH-1251 Add initial implementation of should_use_eos_vm_oc()

025bc22

GH-1251 Disable oc on producer when applying blocks

f2123ea

heifner commented Jun 20, 2023

View reviewed changes

spoonincode reviewed Jun 21, 2023

View reviewed changes

GH-1251 Increase perf harness genesis to have 150ms max transaction t…

9faefa2

…ime to match genesis used in other tests. This allows for setcode in slow ci/cd env.

heifner mentioned this pull request Jun 22, 2023

Add oc-p2p-enable option #1333

Closed

heifner added 2 commits June 22, 2023 08:08

GH-1251 Add additional allowed true/false alternatives

8fc11f0

GH-1251 Add additional comments

8706738

heifner marked this pull request as ready for review June 22, 2023 13:24

heifner requested review from linh2931 and spoonincode June 22, 2023 13:24

heifner added the OCI Work exclusive to OCI team label Jun 22, 2023

linh2931 reviewed Jun 22, 2023

View reviewed changes

heifner linked an issue Jun 22, 2023 that may be closed by this pull request

EOS VM OC: Implement new Auto OC Mode #1251

Closed

heifner added 3 commits June 22, 2023 14:50

GH-1251 Address peer review comments

98e3b81

GH-1251 Add is_applying_block() method

d4ee46b

GH-1251 Add integration test that verifies auto oc tierup

dc00e24

linh2931 approved these changes Jun 23, 2023

View reviewed changes

GH-1251 Use eosio.token as eosio.system contract can not tierup in ti…

6008c72

…me to be found in logs on slow ci/cd machine

spoonincode reviewed Jun 28, 2023

View reviewed changes

heifner added 6 commits June 28, 2023 03:10

GH-1251 No need to link to boost unit_test_framework, using header-only

d373b1d

GH-1251 Update auto description

da236cb

GH-1251 Support case-insensitive options

975ad13

GH-1251 Move setting of eosvmoc_tierup outside of loop

774fd21

GH-1251 Remove improper check for vm_type::eos_vm_oc

b260447

GH-1251 Restore #ifdef EOSIO_EOS_VM_OC_RUNTIME_ENABLED removed in ref…

a8f20ce

…actor

heifner requested a review from spoonincode June 28, 2023 16:49

spoonincode approved these changes Jun 29, 2023

View reviewed changes

heifner added 2 commits June 29, 2023 02:06

GH-1251 Add comment that log statement used in test

7c43e00

GH-1251 Fix tests to not destroy temp dir before controller which use…

cd744f5

…s it

heifner merged commit 91d192b into main Jun 29, 2023

heifner deleted the GH-1251-auto-oc branch June 29, 2023 18:18

	if(eosvmoc_tierup != wasm_interface::vm_oc_enable::oc_none) {
	EOS_ASSERT(vm != wasm_interface::vm_type::eos_vm_oc, wasm_exception, "You can't use EOS VM OC as the base runtime when tier up is activated");
	eosvmoc.emplace(data_dir, eosvmoc_config, d);
	}

		@@ -0,0 +1,2 @@
		#define BOOST_TEST_MODULE state_history_plugin
		#include <boost/test/included/unit_test.hpp> No newline at end of file

Conversation

heifner commented Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Short Description

What

Why

Auto OC Mode as default

Baseline / OC selection

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoonincode left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

heifner commented Jun 20, 2023 •

edited

Loading