Remove stack limit #87

larryk85 · 2019-09-17T00:17:53Z

No description provided.

…en indexing out of the range of the vector

swatanabe-b1 · 2019-09-17T13:10:54Z

include/eosio/vm/vector.hpp

+   class unmanaged_vector {
+      public:
+         constexpr unmanaged_vector(size_t size=0) : _data(size) {}
+         constexpr unmanaged_vector(const unmanaged_vector& v) = delete;


You don't need to delete the copy constructor explicitly, when you define a move constructor.

swatanabe-b1 · 2019-09-17T13:11:41Z

include/eosio/vm/vector.hpp

+      public:
+         constexpr unmanaged_vector(size_t size=0) : _data(size) {}
+         constexpr unmanaged_vector(const unmanaged_vector& v) = delete;
+         constexpr unmanaged_vector(unmanaged_vector&& v) : _data(std::move(v._data)), _index(v._index) {}


swatanabe-b1 · 2019-09-17T13:11:59Z

include/eosio/vm/vector.hpp

+            _data = std::move(v._data);
+            _index = v._index;
+            return *this;
+         }


swatanabe-b1 · 2019-09-17T13:16:53Z

include/eosio/vm/vector.hpp

+         }
+
+         constexpr inline void resize( size_t size ) {
+            _data.resize(size);


This can break the required invariant that _index < _data.size()

swatanabe-b1 · 2019-09-17T13:17:19Z

include/eosio/vm/vector.hpp

+         }
+
+         constexpr inline void back() {
+            return _data[_index];


Off by one.

swatanabe-b1 · 2019-09-17T13:21:39Z

include/eosio/vm/vector.hpp

+         constexpr inline T& at( size_t i ) {
+            // TODO: fix this to be more robust, assuming that the read 'i' will be in close to the max index
+            // this is the usage pattern currently by eos-vm, a better solution will be created for the abstraction
+            // of the various points of validation


Does eos-vm ever access after the current end? The fact the this has different behavior from the const version is scary.

swatanabe-b1 · 2019-09-17T13:22:59Z

include/eosio/vm/vector.hpp

@@ -56,7 +118,7 @@ namespace eosio { namespace vm {
         }

         constexpr inline void pop_back() {
-            EOS_VM_ASSERT( _index >= 0, wasm_vector_oob_exception, "vector pop out of bounds" );
+            EOS_VM_ASSERT( _index-- > 0, wasm_vector_oob_exception, "vector pop out of bounds" );


This has the same issue as in unmanaged_vector.

swatanabe-b1 · 2019-09-17T13:28:17Z

include/eosio/vm/wasm_stack.hpp

+         using type = unmanaged_vector<Elem>;
+      };
+
+      using base_data_store_t = typename base_data_store<ElemT, Allocator>::type;


using base_data_store_t = std::conditional<std::is_same_v<Allocator, nullptr_t>, unmanaged_vector<Elem>, managed_vector<ElemT, Allocator>>;

swatanabe-b1 · 2019-09-17T13:29:47Z

tests/utils.hpp

@@ -50,7 +50,7 @@ inline bool check_nan(const std::optional<eosio::vm::operand_stack_elem>& v) {
 }

 #define BACKEND_TEST_CASE(name, tags) \
-  TEMPLATE_TEST_CASE(name, tags, eosio::vm::interpreter, eosio::vm::jit)
+  TEMPLATE_TEST_CASE(name, tags, eosio::vm::jit, eosio::vm::interpreter)


Any particular reason for this?

No, it was just there from oddness in unit tests.

swatanabe-b1 · 2019-09-17T13:32:39Z

include/eosio/vm/vector.hpp

+
+      private:
+         std::vector<T> _data;
+         size_t _index = 0;


I find this whole _index business confusing. vector has size and capacity. We add an _index on top of that here. Then stack adds another _index on top of that.

swatanabe-b1 · 2019-09-18T14:16:51Z

include/eosio/vm/wasm_stack.hpp

+      using base_data_store_t = typename base_data_store<ElemT, Allocator>::type;
+
+      base_data_store_t _store;
+      uint16_t          _index = 0;


This imposes a limit of 65536 elements, which will silently wrap instead of causing an error.

swatanabe-b1 · 2019-09-19T15:21:35Z

I'm very suspicious of the claim that vector's allocation is too slow. You're reserving an amount equal to the previous hard limit. i.e. it should never need to reallocate on normal usage, so the performance of allocation should be insignificant.

larryk85 · 2019-09-19T17:08:48Z

I'm very suspicious of the claim that vector's allocation is too slow. You're reserving an amount equal to the previous hard limit. i.e. it should never need to reallocate on normal usage, so the performance of allocation should be insignificant.

I should be, but it wasn't. Given the wasm benchmarks the average overhead over develop is about 45-50%, so high enough that we need to do something different. I tried quite a few different ways of using std::vector and the 45-50% overhead was the best I could get it, some ways lead to 80-90% overhead.

swatanabe-b1

I tried quite a few different ways of using std::vector and the 45-50% overhead was the best I could get it, some ways lead to 80-90% overhead.

I don't know exactly what you tried, but e8554f5 is severely broken, because it is confused about vector size vs. capacity.

swatanabe-b1 · 2019-09-19T17:11:37Z

include/eosio/vm/vector.hpp

+
+            constexpr inline void emplace_back( T&& val ) {
+               EOS_VM_ASSERT( _index < _size, wasm_vector_oob_exception, "vector write out of bounds" );
+               _data[_index++] = std::forward<T>(val);


std::forward is a bit pointless, given that this implementation requires T to have a trivial default constructor/copy constructor/destructor.

I'm aware, I didn't push up a lot of different tries because of the excess overhead.

swatanabe-b1 · 2019-09-20T14:08:53Z

I'm not seeing that std::vector is slower. In fact, simply replacing unmanaged_vector with std::vector at the current commit seems to make it faster.

swatanabe-b1 · 2019-09-23T13:19:10Z

tests/stack_restriction_tests.cpp

+   i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64) i32.const 0 call 0) (memory
+   (;0;) 0) (export "fun" (func 1)))
+   */
+   std::vector<uint8_t> __8k_stack_size_1_under_wasm = {


This is a reserved identifier.

…ove_stack_limit

larryk85 · 2019-09-23T20:03:03Z

I'm not seeing that std::vector is slower. In fact, simply replacing unmanaged_vector with std::vector at the current commit seems to make it faster.

Hmmm, you are right. On Linux I wasn't seeing a benefit (they were close enough together), but on Mac there is a small benefit (maybe 5% faster) with std::vector.

swatanabe-b1

It looks fine except for the test issues.

swatanabe-b1 · 2019-09-23T20:45:08Z

tests/vector_tests.cpp

+      unmanaged_vector<char> uv0(10);
+      for (int i = 0; i < uv0.size(); i++)
+         uv0[i] = 'a';
+      CHECK_THROWS_AS([&](){ uv0[10] = 'a'; }(), std::exception);


This fails, since unmanaged_vector is std::vector.

tests/stack_restriction_tests.cpp

…ove_stack_limit

Bucky Kittinger added 12 commits September 12, 2019 15:48

fixing up stack

db1190b

more changes to stack limit, still broken I believe

0b1921f

more changes, still bus error

f153390

created wrapper unmanaged vector to automatically increase in size wh…

b87793c

…en indexing out of the range of the vector

remove forgotten cout

cd91307

Merge remote-tracking branch 'origin/jit-stack' into remove_stack_limit

0fc5ff2

more changes

c6c8ccf

Merge remote-tracking branch 'origin/jit-stack' into remove_stack_limit

42382d8

Merge remote-tracking branch 'origin/jit-stack' into remove_stack_limit

c2b6c1f

host functions tests still failing

5aa2bb6

fix unmanaged_vector and stack_restriction_tests

551f824

revert commented out sections

e3eab10

larryk85 requested a review from swatanabe-b1 September 17, 2019 00:17

resize to at least 1 element if initial size is zero

7882dca

swatanabe-b1 suggested changes Sep 17, 2019

View reviewed changes

swatanabe-b1 reviewed Sep 18, 2019

View reviewed changes

Bucky Kittinger added 4 commits September 18, 2019 19:53

still having issues, pushing to work from home (compile time nightmares)

e82f0ca

oops, wrong last commit, still a lot of overhead

e8554f5

trying another allocator, vector incurs too high an overhead

85d7977

more changes

7139e49

swatanabe-b1 reviewed Sep 19, 2019

View reviewed changes

more changes

39d4c6b

Bucky Kittinger added 2 commits September 20, 2019 18:23

finishing up

620129e

Merge branch 'develop' into remove_stack_limit

f06878d

swatanabe-b1 reviewed Sep 23, 2019

View reviewed changes

Bucky Kittinger added 2 commits September 23, 2019 15:57

change out for std::vector

a1af38a

Merge branch 'remove_stack_limit' of github.com:eosio/eos-vm into rem…

2302582

…ove_stack_limit

Bucky Kittinger and others added 2 commits September 23, 2019 15:59

bad xxd foo

68f6486

Merge branch 'develop' into remove_stack_limit

eb07cad

swatanabe-b1 approved these changes Sep 23, 2019

View reviewed changes

Bucky Kittinger added 3 commits September 23, 2019 16:58

missing include

5c4bfc4

Merge branch 'remove_stack_limit' of github.com:eosio/eos-vm into rem…

ba2e218

…ove_stack_limit

fix tests

1bbc269

swatanabe-b1 merged commit d092086 into develop Sep 23, 2019

swatanabe-b1 deleted the remove_stack_limit branch September 23, 2019 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove stack limit #87

Remove stack limit #87

larryk85 commented Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

larryk85 Sep 17, 2019

swatanabe-b1 Sep 17, 2019

swatanabe-b1 Sep 18, 2019 •

edited

Loading

swatanabe-b1 commented Sep 19, 2019

larryk85 commented Sep 19, 2019

swatanabe-b1 left a comment

swatanabe-b1 Sep 19, 2019

larryk85 Sep 19, 2019

swatanabe-b1 commented Sep 20, 2019

swatanabe-b1 Sep 23, 2019

larryk85 commented Sep 23, 2019

swatanabe-b1 left a comment

swatanabe-b1 Sep 23, 2019

Remove stack limit #87

Remove stack limit #87

Conversation

larryk85 commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swatanabe-b1 Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

swatanabe-b1 commented Sep 19, 2019

larryk85 commented Sep 19, 2019

swatanabe-b1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swatanabe-b1 commented Sep 20, 2019

Choose a reason for hiding this comment

larryk85 commented Sep 23, 2019

swatanabe-b1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swatanabe-b1 Sep 18, 2019 •

edited

Loading