Implement non-copyable block #1542

nickaleks · 2018-07-05T15:14:05Z

Description of the Change

This Pull Request refactors proto block to be non-copy constructible. Adds move semantics for the block.

Benefits

Block no longer can be copied. Additionally, moving a block is faster

Possible Drawbacks

Cannot copy blocks

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

…/noncopyable_block

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

…e_block

This prevents initialization bug from old versions of gcc (pre 7.2) Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

It enforces no-copy for all derived classes Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

muratovv · 2018-07-10T12:07:15Z

CMakeLists.txt

@@ -110,6 +110,8 @@ include_directories(

 SET(IROHA_ROOT_PROJECT ON)

+add_definitions(-DBOOST_NO_RTTI)


Pls, add a comment with the purpose of usage of the flag.

muratovv · 2018-07-10T12:12:48Z

irohad/ametsuchi/impl/postgres_block_query.cpp

            return rxcpp::observable<>::create<PostgresBlockQuery::wBlock>(
-                [block{std::move(block)}](const auto &s) {
-                  if (block) {
+                [i, this](const auto &s) {


maybe rename i and s with some more meaningful names?

i is just a counter so i think its fine. s will be renamed

muratovv · 2018-07-10T12:16:08Z

shared_model/backend/protobuf/CMakeLists.txt

@@ -1,4 +1,5 @@
 add_library(shared_model_proto_backend
+    block.cpp


I think, block.cpp should be moved to impl/ folder regards to the rest files in the target.

muratovv · 2018-07-10T13:44:08Z

shared_model/backend/protobuf/common_objects/noncopyable_proto.hpp

+ public:
+  using TransportType = Proto;
+
+  template <typename Transport>


Add use case of ctor. The documentation should contain information about copy/move semantic of ctor.

muratovv · 2018-07-10T13:49:58Z

shared_model/builders/protobuf/unsigned_proto.hpp

@@ -63,7 +66,7 @@ namespace shared_model {
        if (boost::size(object_.signatures()) == 0) {
          throw std::invalid_argument("Cannot get object without signatures");
        }
-        return object_;
+        return std::move(object_);


I am not sure about the safety of this method because its class may be in bindings. So, here we have to care about signAndAddSignature method and check that this method works correctly.

muratovv · 2018-07-10T14:01:40Z

test/benchmark/bm_proto_creation.cpp

+  st.SetIterationTime(elapsed_seconds.count());
+}
+
+BENCHMARK_DEFINE_F(BlockBenchmark, TransportCopyTest)(benchmark::State &st) {


Add documentation about a semantic of the performance test. Also, this issue is related to tests below.

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

muratovv · 2018-07-12T11:44:03Z

shared_model/builders/protobuf/unsigned_proto.hpp

-     * NOTE: finish() moves internal object, so it is unsafe to call it more
-     * than once on the single instance.
+     * NOTE: finish() moves internal object, so calling methods after
+     * finish() throws an exception
     */
    template <typename T>
    class UnsignedWrapper {


Pls, add move ctor for the class.

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

lebdron · 2018-07-12T18:57:21Z

irohad/ametsuchi/impl/postgres_block_query.cpp

-                        block.value()));
-                  }
-                  s.on_completed();
+                [i, this](const auto &block_observer) {


http://reactivex.io/RxCpp/classrxcpp_1_1observable_3_01void_00_01void_01_4.html#a68356278fbcd12847cec55bcf96a0d00

So block_observer -> subscriber as it seems to me.

lebdron · 2018-07-12T19:19:19Z

test/module/irohad/network/block_loader_test.cpp

-          [](auto &&x) { return wBlock(clone(x)); })));
-  auto block = loader->retrieveBlock(peer_key, kPrevHash);
+      .WillOnce(Return(rxcpp::observable<>::just(wBlock(clone(present)))));
+  auto block = loader->retrieveBlock(peer_key, Hash(std::string(32, '0')));


32 looks like magic, so maybe use kPrevHash?

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

…e_block

sorabot · 2018-07-13T08:12:06Z

SonarQube analysis reported 1 issue

transport_builder_test.cpp#L159: The function 'createInvalidProposal' is never used.

Implement non-copyable block Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

nickaleks added 6 commits July 5, 2018 14:20

implement non-copyable block

492e9ba

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

Add benchmark for copy and move of block and proposal

5faba53

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

Add benchmark for clone of block and proposal

6a58e53

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

Modify benchmark doc to include info about lazy fields

192b1a4

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

Merge branch 'feature/block_proposal_creation_benchmark' into feature…

38276d3

…/noncopyable_block

Fix iroha build with noncopyable block

6fbb357

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

nickaleks requested review from lebdron and muratovv July 5, 2018 15:14

nickaleks added 4 commits July 6, 2018 10:00

Fix move construction benchmark

00da789

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # irohad/ametsuchi/impl/temporary_wsv_impl.cpp

Refactor conflicts

3190fb5

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Refactor bm_proto_creation to perform move correctly

5c3578c

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Add BOOST_NO_RTTI macro

2ef12b2

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

l4l added needs-review pr awaits review from maintainers refactoring internal stuff, that are changed/removed that doesn't affect client code labels Jul 6, 2018

Merge remote-tracking branch 'origin/develop' into feature/noncopyabl…

e29672b

…e_block

nickaleks force-pushed the feature/noncopyable_block branch from b10ed3a to 55deaad Compare July 9, 2018 10:16

Add default constructor for LazyInitalizer

e389a49

This prevents initialization bug from old versions of gcc (pre 7.2) Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

nickaleks force-pushed the feature/noncopyable_block branch from 55deaad to e389a49 Compare July 9, 2018 10:35

Move deleted copy-constructor to NonCopyableProto

d469077

It enforces no-copy for all derived classes Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

muratovv suggested changes Jul 10, 2018

View reviewed changes

Fix review issues

f0072b0

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

nickaleks force-pushed the feature/noncopyable_block branch from 742cc30 to f0072b0 Compare July 12, 2018 08:50

muratovv approved these changes Jul 12, 2018

View reviewed changes

Add move ctor to unsigned wrapper

afb6af6

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

lebdron approved these changes Jul 12, 2018

View reviewed changes

nickaleks added 2 commits July 13, 2018 10:02

Fix review issues

ab13ffb

Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com> # Conflicts: # test/benchmark/CMakeLists.txt # test/benchmark/bm_proto_creation.cpp

Merge remote-tracking branch 'origin/develop' into feature/noncopyabl…

6f1cd24

…e_block

nickaleks merged commit aca7f7f into develop Jul 13, 2018

nickaleks deleted the feature/noncopyable_block branch July 13, 2018 08:35

neewy mentioned this pull request Jul 25, 2018

Hyperledger Iroha v1.0 beta-4 version #1597

Merged

l4l pushed a commit that referenced this pull request Jul 25, 2018

Implement non-copyable block (#1542)

52e7d7b

Implement non-copyable block Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Implement non-copyable block (#1542)

30a89b1

Implement non-copyable block Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Implement non-copyable block (#1542)

24184c9

Implement non-copyable block Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Implement non-copyable block (#1542)

18edb18

Implement non-copyable block Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

l4l pushed a commit that referenced this pull request Jul 25, 2018

Implement non-copyable block (#1542)

6164acc

Implement non-copyable block Signed-off-by: Nikita Alekseev <n.alekseev2104@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement non-copyable block #1542

Implement non-copyable block #1542

nickaleks commented Jul 5, 2018 •

edited

Loading

muratovv Jul 10, 2018

muratovv Jul 10, 2018

nickaleks Jul 12, 2018

muratovv Jul 10, 2018

muratovv Jul 10, 2018

muratovv Jul 10, 2018

muratovv Jul 10, 2018

muratovv Jul 12, 2018

lebdron Jul 12, 2018

lebdron Jul 12, 2018

sorabot commented Jul 13, 2018

		@@ -110,6 +110,8 @@ include_directories(

		SET(IROHA_ROOT_PROJECT ON)

		add_definitions(-DBOOST_NO_RTTI)

		@@ -1,4 +1,5 @@
		add_library(shared_model_proto_backend
		block.cpp

Implement non-copyable block #1542

Implement non-copyable block #1542

Conversation

nickaleks commented Jul 5, 2018 • edited Loading

Description of the Change

Benefits

Possible Drawbacks

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

sorabot commented Jul 13, 2018

nickaleks commented Jul 5, 2018 •

edited

Loading