Size limited containers; Refactored Buffer and BufferView #1373

xDimon · 2022-10-21T06:59:38Z

Signed-off-by: Dmitriy Khaustov aka xDimon [email protected]

Referenced issues

Resolves #1364

Description of the Change

Implemented size limited containers.
Refactored Buffer and BufferView: inheritance instead composition, hold mover span by BufferView

Benefits

Ability to limit size of containers (i.e. can be useful during scale-decode).
Fixed some using memory of just destroyed temporary buffers.

Usage Examples or Tests

Appended

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

codecov · 2022-10-21T07:50:17Z

Codecov Report

Merging #1373 (3fae415) into master (88bfa95) will increase coverage by 0.38%.
The diff coverage is 70.14%.

@@            Coverage Diff             @@
##           master    #1373      +/-   ##
==========================================
+ Coverage   24.19%   24.57%   +0.38%     
==========================================
  Files         628      629       +1     
  Lines       23520    23613      +93     
  Branches    12287    12288       +1     
==========================================
+ Hits         5691     5804     +113     
- Misses      12540    12543       +3     
+ Partials     5289     5266      -23

Impacted Files	Coverage Δ
.../service/child_state/impl/child_state_api_impl.cpp	`22.09% <0.00%> (+2.32%)`	⬆️
core/blockchain/impl/block_storage_impl.cpp	`22.80% <0.00%> (ø)`
core/blockchain/impl/storage_util.cpp	`41.86% <0.00%> (ø)`
core/common/blob.hpp	`63.26% <ø> (ø)`
core/host_api/impl/child_storage_extension.cpp	`26.50% <0.00%> (-0.45%)`	⬇️
core/host_api/impl/storage_extension.cpp	`15.21% <0.00%> (ø)`
core/injector/application_injector.cpp	`32.85% <0.00%> (+0.05%)`	⬆️
core/offchain/impl/offchain_local_storage.cpp	`0.00% <0.00%> (ø)`
core/offchain/impl/offchain_persistent_storage.cpp	`8.57% <0.00%> (ø)`
core/primitives/authority.hpp	`84.61% <ø> (ø)`
... and 23 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

…nditional compiling Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

core/common/buffer_view.hpp

core/common/size_limited_containers.hpp

turuslan · 2022-10-25T11:44:55Z

core/common/size_limited_containers.hpp

+    bool operator==(const Base &other) const noexcept {
+      return Base::size() == other.size()
+             and std::equal(
+                 Base::cbegin(), Base::cend(), other.cbegin(), other.cend());
+    }
+
+    template <size_t Size>
+    bool operator==(const std::array<typename Base::value_type, Size> &other)
+        const noexcept {
+      return Base::size() == Size
+             and std::equal(
+                 Base::cbegin(), Base::cend(), other.cbegin(), other.cend());
+    }
+
+    bool operator<(const Base &other) const noexcept {
+      return std::lexicographical_compare(
+          Base::cbegin(), Base::cend(), other.cbegin(), other.cend());
+    }
+
+    template <size_t Size>
+    bool operator<(const std::array<typename Base::value_type, Size> &other)
+        const noexcept {
+      return std::lexicographical_compare(
+          Base::cbegin(), Base::cend(), other.cbegin(), other.cend());
+    }


May want bidirectional compare like Self < Base and Base < Self.
Maybe boost::equality_comparable, or friend functions.

core/common/size_limited_containers.hpp

core/storage/trie/polkadot_trie/polkadot_trie_impl.cpp

core/common/buffer_view.hpp

core/common/buffer.hpp

core/utils/kagome_db_editor.cpp

test/core/api/service/state/state_api_test.cpp

core/common/buffer.hpp

core/common/buffer_view.hpp

core/common/size_limited_containers.hpp

iceseer · 2022-10-26T13:31:41Z

core/common/size_limited_containers.hpp

+
+    SizeLimitedContainer() = default;
+
+    SizeLimitedContainer(


Я бы оставил только мув-/копи-конструкторы, инит лист, и через итераторы.

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

turuslan · 2022-10-27T11:17:15Z

core/common/buffer.hpp

-
-  template <size_t MaxSize>
-  using SLBufer = BufferT<MaxSize>;
+  typedef SLBuffer<std::numeric_limits<size_t>::max()> Buffer;


Why not using?

core/common/size_limited_containers.hpp

core/common/buffer.hpp

turuslan · 2022-10-27T11:34:53Z

core/common/buffer_back_insert_iterator.hpp


-  back_insert_iterator<Buffer> &operator=(uint8_t value) {
+  back_insert_iterator<Container> &operator=(uint8_t value) {


May shorten

Suggested change

back_insert_iterator<Container> &operator=(uint8_t value) {

using Self = back_insert_iterator<Container>;

Self &operator=(uint8_t value) {

or

Suggested change

back_insert_iterator<Container> &operator=(uint8_t value) {

auto &operator=(uint8_t value) {

turuslan · 2022-10-27T12:09:37Z

core/common/size_limited_containers.hpp

+            return Base(size);
+          }()) {}
+
+    SizeLimitedContainer(size_t size, const typename Base::value_type &value)


Can we simplify typename Base::value_type to
value_type (because we are already inherited that container)
or using?

core/common/buffer.hpp

turuslan · 2022-10-27T13:06:18Z

core/common/buffer.hpp


-  std::ostream &operator<<(std::ostream &os, const Buffer &buffer);
-  std::ostream &operator<<(std::ostream &os, BufferView view);
+  using BufferMutRef = std::reference_wrapper<Buffer>;


Unused

Suggested change

using BufferMutRef = std::reference_wrapper<Buffer>;

turuslan · 2022-10-27T13:07:50Z

core/blockchain/impl/common.cpp

@@ -41,7 +40,8 @@ namespace kagome::blockchain {
    }
    auto root = trie.getRoot();
    if (root == nullptr) {
-      return codec.hash256(common::Buffer{0});
+      static const auto zero_hash = codec.hash256(common::Buffer{0});


Testing mocks may conflict

turuslan · 2022-10-27T13:10:34Z

core/host_api/impl/storage_extension.cpp

@@ -371,7 +371,7 @@ namespace kagome::host_api {
    storage::trie::PolkadotCodec codec;
    if (pv.empty()) {
      static const auto empty_root =
-          common::Buffer{}.put(codec.hash256(common::Buffer{0}));
+          common::Buffer(codec.hash256(common::Buffer{0}));


Should this be moved to global const?

core/common/buffer_view.hpp

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

feature: size limited containers

e56d593

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

xDimon added 5 commits October 25, 2022 14:40

refactor: size limited containers (exception, operators, improvements)

422dc19

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

update: use new scale-codec

ae933da

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

refactor: Buffer and BufferView

59c5821

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: unit tests

4f9d119

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

refactor: improve size limited containers by branch prediction and co…

29440f7

…nditional compiling Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

xDimon marked this pull request as ready for review October 25, 2022 09:02

xDimon requested review from iceseer and kamilsa October 25, 2022 09:03

xDimon changed the title ~~Feature: size limited containers~~ Size limited containers; Refactored Buffer and BufferView Oct 25, 2022

fix: compiling on macos

9338941

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

turuslan reviewed Oct 25, 2022

View reviewed changes

core/common/buffer_view.hpp Outdated Show resolved Hide resolved

turuslan reviewed Oct 25, 2022

View reviewed changes

turuslan requested changes Oct 25, 2022

View reviewed changes

kamilsa reviewed Oct 26, 2022

View reviewed changes

core/common/buffer.hpp Outdated Show resolved Hide resolved

core/common/buffer.hpp Outdated Show resolved Hide resolved

kamilsa approved these changes Oct 26, 2022

View reviewed changes

iceseer reviewed Oct 26, 2022

View reviewed changes

xDimon added 10 commits October 27, 2022 15:53

git: Merge branch 'master' into feature/size_limited_containers

329df61

fix: size limited container interfaces

7ca9122

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

f550fbe

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

ca879ec

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

d020bd9

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

5476c5b

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

c5fc6c3

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

e670451

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

ec623c7

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

fix: review issues

dba09f5

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

turuslan reviewed Oct 27, 2022

View reviewed changes

fix: review issues

ca654aa

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

xDimon added 2 commits October 28, 2022 01:50

fix: review issues

72646e8

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

refactor: replace some vectors by limited version

3022bd5

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

xDimon requested review from turuslan and iceseer October 27, 2022 18:09

xDimon added 2 commits October 28, 2022 15:02

update: hunter

45e1322

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

clean: remove custom config

3fae415

Signed-off-by: Dmitriy Khaustov aka xDimon <[email protected]>

turuslan approved these changes Oct 28, 2022

View reviewed changes

xDimon merged commit db19edf into master Oct 28, 2022

xDimon deleted the feature/size_limited_containers branch October 28, 2022 10:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Size limited containers; Refactored Buffer and BufferView #1373

Size limited containers; Refactored Buffer and BufferView #1373

xDimon commented Oct 21, 2022 •

edited

Loading

codecov bot commented Oct 21, 2022 •

edited

Loading

turuslan Oct 25, 2022 •

edited

Loading

iceseer Oct 26, 2022

turuslan Oct 27, 2022

turuslan Oct 27, 2022

turuslan Oct 27, 2022 •

edited

Loading

turuslan Oct 27, 2022

turuslan Oct 27, 2022

turuslan Oct 27, 2022


		back_insert_iterator<Buffer> &operator=(uint8_t value) {
		back_insert_iterator<Container> &operator=(uint8_t value) {

	back_insert_iterator<Container> &operator=(uint8_t value) {
	using Self = back_insert_iterator<Container>;
	Self &operator=(uint8_t value) {

	back_insert_iterator<Container> &operator=(uint8_t value) {
	auto &operator=(uint8_t value) {

Size limited containers; Refactored Buffer and BufferView #1373

Size limited containers; Refactored Buffer and BufferView #1373

Conversation

xDimon commented Oct 21, 2022 • edited Loading

Referenced issues

Description of the Change

Benefits

Usage Examples or Tests

codecov bot commented Oct 21, 2022 • edited Loading

Codecov Report

turuslan Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

iceseer Oct 26, 2022

Choose a reason for hiding this comment

turuslan Oct 27, 2022

Choose a reason for hiding this comment

turuslan Oct 27, 2022

Choose a reason for hiding this comment

turuslan Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

turuslan Oct 27, 2022

Choose a reason for hiding this comment

turuslan Oct 27, 2022

Choose a reason for hiding this comment

turuslan Oct 27, 2022

Choose a reason for hiding this comment

xDimon commented Oct 21, 2022 •

edited

Loading

codecov bot commented Oct 21, 2022 •

edited

Loading

turuslan Oct 25, 2022 •

edited

Loading

turuslan Oct 27, 2022 •

edited

Loading