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

SDSTOR-10892 : concurrent mem btree UT #137

Merged
merged 1 commit into from
Sep 18, 2023
Merged

SDSTOR-10892 : concurrent mem btree UT #137

merged 1 commit into from
Sep 18, 2023

Conversation

shosseinimotlagh
Copy link
Contributor

No description provided.

return s_ptr;
//static boost::fibers::local_fiber <BtreeThreadVariables> *s_ptr{nullptr};
auto this_id( boost::this_fiber::get_id() );
static thread_local std::map<fibers::fiber::id, std::unique_ptr<BtreeThreadVariables>> fiber_map;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this assume that fibers are not moved across threads. In that case fiber1 will have entry in multiple threads.

}

void random_get() {
static std::uniform_int_distribution< uint32_t > s_rand_range_generator{1, 100};
Copy link
Contributor

Choose a reason for hiding this comment

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

this has to be thread_local or thread safe. as internal state is changed. same in all functions.

src/tests/test_mem_btree.cpp Outdated Show resolved Hide resolved
src/tests/test_mem_btree.cpp Outdated Show resolved Hide resolved
key = m_range_scheduler.pick_random_non_existing_keys(nkeys, m_max_range_input);
}

if (key == -1) { return; }
Copy link
Contributor

Choose a reason for hiding this comment

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

assert ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no assert is needed. It is possible that in this range, there is no available free keys (or after multiple random attempts) . So lets return it.

}

private:
bool non_of_them_existed(uint32_t key, uint32_t nkeys) {
Copy link
Contributor

Choose a reason for hiding this comment

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

std::none_of ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as nit: std::all_of

src/tests/test_mem_btree.cpp Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2023

Codecov Report

Patch coverage: 67.85% and project coverage change: +7.20% 🎉

Comparison is base (b6c6a89) 47.56% compared to head (e970145) 54.77%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   47.56%   54.77%   +7.20%     
==========================================
  Files          86       87       +1     
  Lines        7114     7285     +171     
  Branches      916      947      +31     
==========================================
+ Hits         3384     3990     +606     
+ Misses       3403     2894     -509     
- Partials      327      401      +74     
Files Changed Coverage Δ
...clude/homestore/btree/detail/btree_mutate_impl.ipp 54.35% <ø> (+1.26%) ⬆️
src/include/homestore/btree/detail/varlen_node.hpp 75.46% <50.00%> (+7.72%) ⬆️
src/include/homestore/btree/detail/simple_node.hpp 73.15% <55.55%> (+15.09%) ⬆️
...clude/homestore/btree/detail/btree_remove_impl.ipp 51.05% <83.33%> (-5.77%) ⬇️
src/include/homestore/btree/btree.hpp 100.00% <100.00%> (ø)

... and 34 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -37,6 +37,12 @@ typedef std::function< bool(const BtreeKey&, const BtreeKey&, const BtreeValue&,

using BtreeNodePtr = boost::intrusive_ptr< BtreeNode >;

struct BtreeVisualizeVariables {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets keep this in a separate CL.

if (s_ptr == nullptr) {
static thread_local BtreeThreadVariables inst;
s_ptr = &inst;
#if 0
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this ?

@@ -327,11 +327,12 @@ void Btree< K, V >::print_tree(const std::string& file) const {
to_string(m_root_node_info.bnode_id(), buf);
m_btree_lock.unlock_shared();

BT_LOG(INFO, "Pre order traversal of tree:\n<{}>", buf);
Copy link
Contributor

Choose a reason for hiding this comment

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

we can log the btree to 2 places stdout/BT_LOG and file. file is mostly just for testing.

@@ -345,6 +346,46 @@ void Btree< K, V >::print_tree_keys() const {
BT_LOG(INFO, "Pre order traversal of tree:\n<{}>", buf);
}

template < typename K, typename V >
Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a separate CL for the visualization

@@ -46,7 +46,7 @@ chunk_selector_type_t test_common::HSTestHelper::s_ds_chunk_sel_type;

SISL_OPTION_GROUP(test_index_btree,
(num_iters, "", "num_iters", "number of iterations for rand ops",
::cxxopts::value< uint32_t >()->default_value("65536"), "number"),
::cxxopts::value< uint32_t >()->default_value("1500"), "number"),
Copy link
Contributor

Choose a reason for hiding this comment

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

with 4k , 65k entries should work right ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct. considering 4 btree node variations, this is for building purpose to make it fast.

sanebay
sanebay previously approved these changes Sep 16, 2023
@shosseinimotlagh shosseinimotlagh merged commit c989116 into eBay:master Sep 18, 2023
16 checks passed
@shosseinimotlagh shosseinimotlagh deleted the SDSTOR-10892 branch November 13, 2023 22:54
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