Skip to content
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

[4.0] Added missing calls to wasmifs on read-only threads #964

Merged
merged 6 commits into from
Apr 5, 2023

Conversation

linh2931
Copy link
Member

@linh2931 linh2931 commented Apr 4, 2023

Resolve #963

  • Added missing calls to wasmifs on read-only threads
  • Removed first_block_num_used from wasm_instantiation_cache.emplace
  • Replaced thread_local for wasmifs with a local variable (unordered_map)

@linh2931 linh2931 requested review from heifner and spoonincode April 4, 2023 18:57
@@ -46,6 +46,6 @@ namespace eosio { namespace chain { namespace webassembly {
}

void interface::eosio_exit( int32_t code ) const {
context.control.get_wasm_interface().exit();
context.control.wasm_interface_exit();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding right, I don't think this is the correct behavior. When a contract calls eosio_exit it's a "clean" exit of the executing contract. But this now calls

void wasm_interface_exit() {
// exit all running wasmifs
wasmif.exit();
for (auto& ele: threaded_wasmifs) {
ele.second->exit();
}
}

which looks like it operates on all executing contracts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Matt. You are correct. I was wrong to exit other executing contracts. Am reverting it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am looking into #956 after this PR.

@heifner
Copy link
Member

heifner commented Apr 5, 2023

This static map causes instability:

static std::map<Key,FunctionType*> map;

Here is how I fixed it:

diff --git a/libraries/wasm-jit/Source/IR/Types.cpp b/libraries/wasm-jit/Source/IR/Types.cpp
index d1ade9c3b1..ff3704fd67 100644
--- a/libraries/wasm-jit/Source/IR/Types.cpp
+++ b/libraries/wasm-jit/Source/IR/Types.cpp
@@ -1,6 +1,7 @@
 #include "Types.h"

 #include <map>
+#include <mutex>

 namespace IR
 {
@@ -25,6 +26,8 @@ namespace IR
        template<typename Key,typename Value,typename CreateValueThunk>
        Value findExistingOrCreateNew(std::map<Key,Value>& map,Key&& key,CreateValueThunk createValueThunk)
        {
+      static std::mutex mtx;
+      std::lock_guard<std::mutex> g(mtx);
                auto mapIt = map.find(key);
                if(mapIt != map.end()) { return mapIt->second; }
                else

There is tabs in that file, so the indentation looks weird in the diff.

Copy link
Member

@heifner heifner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix use of static map

@spoonincode
Copy link
Member

@heifner that maybe should be another issue; though it wouldn't surprise me if it's related to #969 since what is pointed to in there may be the root cause of WAVM code being touched from multiple threads.

Copy link
Member

@heifner heifner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved assuming that the static map will be fixed in a diff PR.

@linh2931
Copy link
Member Author

linh2931 commented Apr 5, 2023

I am doing a separate PR for #969 shortly.

@linh2931
Copy link
Member Author

linh2931 commented Apr 5, 2023

The PR is #975

@heifner
Copy link
Member

heifner commented Apr 5, 2023

This static map causes instability:

Verified that #975 fixes this issue.

@linh2931
Copy link
Member Author

linh2931 commented Apr 5, 2023

Thanks @heifner and @spoonincode!

Copy link
Member

@spoonincode spoonincode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

wasm_interface wasmif; // used by main thread and all threads for EOSVMOC
thread_local static std::unique_ptr<wasm_interface> wasmif_thread_local; // a copy for each read-only thread, used by eos-vm and eos-vm-jit
std::mutex threaded_wasmifs_mtx;
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure you need a unique_ptr here.. couldn't you just emplace() the wasm_interface in?

@linh2931 linh2931 merged commit bc34968 into release/4.0 Apr 5, 2023
@linh2931 linh2931 deleted the wasmif_methods_4_0 branch April 5, 2023 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Some wasm_interface methods not called by wamsifs on read-only threads
3 participants