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

Fix upper bound in cursor and remove in topper batch #1180

Merged
merged 9 commits into from
Apr 27, 2022

Conversation

Harrm
Copy link
Contributor

@Harrm Harrm commented Apr 20, 2022

Referenced issues

related to #942

Description of the Change

  • Fix upper bound search algorithm. Details are in the comment for testcase.
  • Fix remove in topper batch. It ignored removal if it didn't contain the key in its own cache, while it should've passed it to the underlying batch.

Benefits

It works.

Possible Drawbacks

No funny debugging anymore.

Usage Examples or Tests

Provided in the code.

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #1180 (dec3a2e) into master (801acdb) will decrease coverage by 0.02%.
The diff coverage is 16.88%.

❗ Current head dec3a2e differs from pull request most recent head df524ff. Consider uploading reports for the commit df524ff to get more accurate results

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
- Coverage   27.84%   27.81%   -0.03%     
==========================================
  Files         553      553              
  Lines       19454    19473      +19     
  Branches    10069    10085      +16     
==========================================
  Hits         5417     5417              
- Misses       9211     9215       +4     
- Partials     4826     4841      +15     
Impacted Files Coverage Δ
...ore/authorship/impl/block_builder_factory_impl.cpp 50.00% <ø> (ø)
core/host_api/impl/child_storage_extension.cpp 26.94% <ø> (ø)
core/host_api/impl/storage_extension.cpp 13.37% <0.00%> (-0.29%) ⬇️
core/runtime/common/trie_storage_provider_impl.cpp 30.66% <0.00%> (-2.20%) ⬇️
...re/storage/trie/impl/ephemeral_trie_batch_impl.cpp 34.37% <ø> (ø)
core/storage/trie/impl/topper_trie_batch_impl.cpp 18.18% <0.00%> (-1.05%) ⬇️
...e/trie/polkadot_trie/polkadot_trie_cursor_impl.cpp 40.94% <0.00%> (-0.91%) ⬇️
core/storage/trie/serialization/polkadot_codec.cpp 36.73% <0.00%> (-1.28%) ⬇️
core/storage/trie/serialization/polkadot_codec.hpp 100.00% <ø> (ø)
.../storage/trie/polkadot_trie/polkadot_trie_impl.cpp 47.29% <7.69%> (-1.40%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 801acdb...df524ff. Read the comment docs.

@Harrm Harrm requested review from sanblch and igor-egorov April 22, 2022 08:51
@@ -25,7 +25,9 @@ namespace kagome::authorship {
outcome::result<std::unique_ptr<BlockBuilder>> BlockBuilderFactoryImpl::make(
const kagome::primitives::BlockInfo &parent,
primitives::Digest inherent_digest) const {
#ifndef BOOST_ASSERT_IS_VOID
Copy link
Contributor

Choose a reason for hiding this comment

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

What is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is defined when BOOST_ASSERT is compiled to noop.

@@ -212,7 +207,7 @@ namespace kagome::host_api {
if (limit_opt) {
SL_TRACE_VOID_FUNC_CALL(logger_, prefix, limit_opt.value());
} else {
SL_TRACE_VOID_FUNC_CALL(logger_, prefix, std::string_view{"none"});
SL_TRACE_VOID_FUNC_CALL(logger_, prefix, std::string_view {"none"});
Copy link
Contributor

Choose a reason for hiding this comment

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

the file missing autoformat

Copy link
Contributor

@igor-egorov igor-egorov left a comment

Choose a reason for hiding this comment

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

looks legit

@Harrm Harrm merged commit 778a051 into master Apr 27, 2022
@Harrm Harrm deleted the fix/cursor-upper-bound branch April 27, 2022 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants