mpsc_queue: allow interrupting try_consume_batch by igchor · Pull Request #1104 · pmem/libpmemobj-cpp

igchor · 2021-06-28T12:11:11Z

If interrupt (exception) happens, next try_consume_batch will
resume from the previous position.

This change is

codecov · 2021-06-28T13:04:02Z

Codecov Report

Merging #1104 (de6c200) into master (fd6e426) will increase coverage by 0.09%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
+ Coverage   94.14%   94.24%   +0.09%     
==========================================
  Files          51       51              
  Lines        5227     5247      +20     
==========================================
+ Hits         4921     4945      +24     
+ Misses        306      302       -4

Flag	Coverage Δ
tests_clang_debug_cpp17	`93.81% <100.00%> (+0.10%)`	⬆️
tests_gcc_debug	`90.69% <88.23%> (+0.11%)`	⬆️

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

Impacted Files	Coverage Δ
include/libpmemobj++/experimental/mpsc_queue.hpp	`94.47% <100.00%> (+0.65%)`	⬆️
include/libpmemobj++/experimental/radix_tree.hpp	`98.51% <0.00%> (+0.14%)`	⬆️
...ude/libpmemobj++/container/concurrent_hash_map.hpp	`94.20% <0.00%> (+0.15%)`	⬆️
include/libpmemobj++/container/vector.hpp	`92.95% <0.00%> (+0.33%)`	⬆️
include/libpmemobj++/transaction.hpp	`80.64% <0.00%> (+0.80%)`	⬆️

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 fd6e426...1f4030c. Read the comment docs.

lukaszstolarczuk

Reviewed 3 of 5 files at r1.
Reviewable status: 3 of 5 files reviewed, 4 unresolved discussions (waiting on @igchor and @karczex)

include/libpmemobj++/experimental/mpsc_queue.hpp, line 83 at r1 (raw file):

	pmem_log_type *pmem;

	static constexpr size_t consume_invalid =

what exactly these vars are storing...?

tests/CMakeLists.txt, line 973 at r1 (raw file):

	build_test(mpsc_queue_consume_interrupt mpsc_queue/consume_interrupt.cpp)
	add_test_generic(NAME mpsc_queue_consume_interrupt SCRIPT mpsc_queue/mpsc_queue_consume_interrupt_0.cmake TRACERS none memcheck pmemcheck)

_0 in script's name is redundant, I believe

tests/mpsc_queue/consume_interrupt.cpp, line 5 at r1 (raw file):

/*
 * basic.pp -- Single threaded tests for

wrong name and description 😉

tests/mpsc_queue/consume_interrupt.cpp, line 17 at r1 (raw file):

#include <libpmemobj++/make_persistent.hpp>
#include <libpmemobj++/persistent_ptr.hpp>
#include <libpmemobj++/string_view.hpp>

it seems redundant

igchor

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

include/libpmemobj++/experimental/mpsc_queue.hpp, line 83 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

what exactly these vars are storing...?

Done.

tests/CMakeLists.txt, line 973 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

_0 in script's name is redundant, I believe

Done.

tests/mpsc_queue/consume_interrupt.cpp, line 5 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

wrong name and description 😉

Done.

tests/mpsc_queue/consume_interrupt.cpp, line 17 at r1 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

it seems redundant

Done.

lukaszstolarczuk

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @igchor and @karczex)

include/libpmemobj++/experimental/mpsc_queue.hpp, line 87 at r2 (raw file):

	/* Stores offset and length of next message to be consumed. If
	 * The values are set to `consume_invalid` next message must be

the values (lowercase)

and what exactly did you mean by "if the values are set"?

include/libpmemobj++/experimental/mpsc_queue.hpp, line 88 at r2 (raw file):

	/* Stores offset and length of next message to be consumed. If
	 * The values are set to `consume_invalid` next message must be
	 * obtained from ringbug_consume. */

this ringbuf is bugged 😉 (ringbug)

tests/mpsc_queue/consume_interrupt.cpp, line 49 at r2 (raw file):

		UT_ASSERT(!ret);

		/* XXX: this is to make sure that try_consume_batch later in the

why is this an XXX ?

igchor

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

include/libpmemobj++/experimental/mpsc_queue.hpp, line 87 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

the values (lowercase)

and what exactly did you mean by "if the values are set"?

I mean if someone sets them to consume_invalid ;d I'm not sure how to reprehase it

include/libpmemobj++/experimental/mpsc_queue.hpp, line 88 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

this ringbuf is bugged 😉 (ringbug)

Done.

tests/mpsc_queue/consume_interrupt.cpp, line 49 at r2 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

why is this an XXX ?

Because we want to eventually hide the wraparound behind the iterators. So try_consume_batch will call the callback only once.

karczex

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

include/libpmemobj++/experimental/mpsc_queue.hpp, line 286 at r3 (raw file):

		 * try_consume_batch failed. In that case, call f() with the
		 * same offset/len as before. */
		if (consume_offset == consume_invalid) {

Wouldn't be better to add to ringbuff_t varialble which track state of the consume (as may be only one consumer, the change would be trivial)

bool consume_pending;

which you set at the beginning of ringbuf_consume and reset at the end of ringbuf_release.
And early return 0 from ringbuf_consume if previous one is pending.

So instead of this comparison to consume_invalid, you may check

if(ring_buffer->consume_pending)

igchor

Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @karczex and @lukaszstolarczuk)

include/libpmemobj++/experimental/mpsc_queue.hpp, line 286 at r3 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

Wouldn't be better to add to ringbuff_t varialble which track state of the consume (as may be only one consumer, the change would be trivial)
bool consume_pending;
which you set at the beginning of ringbuf_consume and reset at the end of ringbuf_release.
And early return 0 from ringbuf_consume if previous one is pending.

So instead of this comparison to consume_invalid, you may check
if(ring_buffer->consume_pending) 

Done.

karczex

Reviewable status: 3 of 6 files reviewed, 3 unresolved discussions (waiting on @lukaszstolarczuk)

lukaszstolarczuk

Reviewed 3 of 3 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @igchor)

If interrupt (exception) happens, next try_consume_batch will resume from the previous position.

lukaszstolarczuk

Reviewed 1 of 1 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @igchor)

igchor requested a review from karczex June 28, 2021 12:11

lukaszstolarczuk reviewed Jun 28, 2021

View reviewed changes

igchor force-pushed the mpsc_queue_consume_interrupt branch from b424d97 to a615b52 Compare June 30, 2021 13:50

igchor commented Jun 30, 2021

View reviewed changes

igchor force-pushed the mpsc_queue_consume_interrupt branch from a615b52 to 7cb14ca Compare July 1, 2021 09:16

lukaszstolarczuk reviewed Jul 1, 2021

View reviewed changes

igchor commented Jul 1, 2021

View reviewed changes

igchor force-pushed the mpsc_queue_consume_interrupt branch from 7cb14ca to de6c200 Compare July 1, 2021 14:29

karczex reviewed Jul 2, 2021

View reviewed changes

igchor force-pushed the mpsc_queue_consume_interrupt branch from de6c200 to 68aedfc Compare July 2, 2021 10:28

igchor commented Jul 2, 2021

View reviewed changes

karczex reviewed Jul 2, 2021

View reviewed changes

lukaszstolarczuk reviewed Jul 2, 2021

View reviewed changes

mpsc_queue: allow interrupting try_consume_batch

1f4030c

If interrupt (exception) happens, next try_consume_batch will resume from the previous position.

igchor force-pushed the mpsc_queue_consume_interrupt branch from 68aedfc to 1f4030c Compare July 2, 2021 11:40

lukaszstolarczuk reviewed Jul 2, 2021

View reviewed changes

lukaszstolarczuk merged commit 2a8e855 into pmem:master Jul 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mpsc_queue: allow interrupting try_consume_batch#1104

mpsc_queue: allow interrupting try_consume_batch#1104
lukaszstolarczuk merged 1 commit intopmem:masterfrom
igchor:mpsc_queue_consume_interrupt

igchor commented Jun 28, 2021 •

edited

Loading

Uh oh!

codecov Bot commented Jun 28, 2021 •

edited

Loading

Uh oh!

lukaszstolarczuk left a comment

Uh oh!

igchor left a comment

Uh oh!

lukaszstolarczuk left a comment

Uh oh!

igchor left a comment

Uh oh!

karczex left a comment

Uh oh!

igchor left a comment

Uh oh!

karczex left a comment

Uh oh!

lukaszstolarczuk left a comment

Uh oh!

lukaszstolarczuk left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

igchor commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jun 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

karczex left a comment

Choose a reason for hiding this comment

Uh oh!

igchor left a comment

Choose a reason for hiding this comment

Uh oh!

karczex left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

lukaszstolarczuk left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

igchor commented Jun 28, 2021 •

edited

Loading

codecov Bot commented Jun 28, 2021 •

edited

Loading