Skip to content
This repository was archived by the owner on Mar 22, 2023. It is now read-only.

Increase ringbuf#1105

Closed
karczex wants to merge 2 commits intopmem:masterfrom
karczex:increase_ringbuf
Closed

Increase ringbuf#1105
karczex wants to merge 2 commits intopmem:masterfrom
karczex:increase_ringbuf

Conversation

@karczex
Copy link
Copy Markdown

@karczex karczex commented Jun 28, 2021

This change is Reviewable

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 28, 2021

Codecov Report

Merging #1105 (61a9f6c) into master (22fa029) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1105      +/-   ##
==========================================
- Coverage   94.23%   94.14%   -0.10%     
==========================================
  Files          51       51              
  Lines        5224     5224              
==========================================
- Hits         4923     4918       -5     
- Misses        301      306       +5     
Flag Coverage Δ
tests_clang_debug_cpp17 93.62% <100.00%> (-0.17%) ⬇️
tests_gcc_debug 90.74% <100.00%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
include/libpmemobj++/detail/ringbuf.hpp 96.92% <100.00%> (ø)
...libpmemobj++/detail/enumerable_thread_specific.hpp 98.91% <0.00%> (-1.09%) ⬇️
include/libpmemobj++/transaction.hpp 79.83% <0.00%> (-0.81%) ⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp 93.88% <0.00%> (-0.32%) ⬇️
include/libpmemobj++/experimental/radix_tree.hpp 98.36% <0.00%> (-0.15%) ⬇️

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 22fa029...61a9f6c. Read the comment docs.

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @karczex)


tests/mpsc_queue/ringbuf.cpp, line 54 at r1 (raw file):

	size_t len, woff;
	ptrdiff_t off;
	std::cout << "test_wraparound " << n << std::endl;

if we want to keep that, pls write it e.g. like: "test_wraparound for n=" << n

Copy link
Copy Markdown
Contributor

@igchor igchor left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karczex)


include/libpmemobj++/detail/ringbuf.hpp, line 71 at r1 (raw file):

typedef uint64_t ringbuf_off_t;

static constexpr ringbuf_off_t RBUF_OFF_MASK = UINT64_MAX >> 8;

Shouldn't you match RBUF_OFF_MASK with WRAP_COUNTER?


tests/mpsc_queue/ringbuf.cpp, line 333 at r1 (raw file):

	test_overlap(24_Peta);

	test_wraparound(100_Giga);

Did those tests pass withouth the ringbuf change?

Copy link
Copy Markdown
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karczex)


include/libpmemobj++/detail/ringbuf.hpp, line 72 at r1 (raw file):

static constexpr ringbuf_off_t RBUF_OFF_MASK = UINT64_MAX >> 8;
static constexpr ringbuf_off_t WRAP_LOCK_BIT = 0x1ULL << 63;

these magic numbers could be actually calculated, I guess


tests/common/size_literals.hpp, line 4 at r1 (raw file):

/* Copyright 2021, Intel Corporation */

#ifndef SIZE_LITERALS_HPP

maybe add LIBPMEMOBJ_CPP_ prefix to this name

@karczex karczex force-pushed the increase_ringbuf branch 2 times, most recently from 0efbbb1 to 2d4ec91 Compare July 2, 2021 06:18
Copy link
Copy Markdown
Author

@karczex karczex left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @igchor, @karczex, and @lukaszstolarczuk)


tests/common/size_literals.hpp, line 4 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

maybe add LIBPMEMOBJ_CPP_ prefix to this name

Done.


tests/mpsc_queue/ringbuf.cpp, line 54 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

if we want to keep that, pls write it e.g. like: "test_wraparound for n=" << n

Done.


tests/mpsc_queue/ringbuf.cpp, line 333 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Did those tests pass withouth the ringbuf change?

no

@karczex karczex force-pushed the increase_ringbuf branch from 2d4ec91 to 61a9f6c Compare July 2, 2021 07:04
@igchor
Copy link
Copy Markdown
Contributor

igchor commented Jul 2, 2021

For now, we'll go with: #1121

@igchor igchor closed this Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants