[FEATURE] view::async_buffer #1205

h-2 · 2019-08-08T16:58:20Z

Enables async I/O even with multiple consumer threads.

Does not yet model View, because it is not Semiregular. I don't mind merging this now and adapting the tests when range-v3 is brought up to date.

TODO:

add changelog

h-2 · 2019-08-09T09:25:26Z

failed macOS tests are due to #1206

include/seqan3/range/view/async_buffer.hpp

test/unit/range/view/view_async_buffer_test.cpp

include/seqan3/range/view/async_buffer.hpp

codecov · 2019-08-12T13:14:23Z

Codecov Report

Merging #1205 into master will increase coverage by <.01%.
The diff coverage is 98.24%.

@@            Coverage Diff             @@
##           master    #1205      +/-   ##
==========================================
+ Coverage   96.83%   96.84%   +<.01%     
==========================================
  Files         218      219       +1     
  Lines        8748     8801      +53     
==========================================
+ Hits         8471     8523      +52     
- Misses        277      278       +1

Impacted Files	Coverage Δ
...de/seqan3/range/detail/inherited_iterator_base.hpp	`90% <100%> (+0.71%)`	⬆️
include/seqan3/range/view/async_input_buffer.hpp	`98.14% <98.14%> (ø)`
include/seqan3/range/view/take.hpp	`97.91% <0%> (-0.13%)`	⬇️
...etail/alignment_matrix_column_major_range_base.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/range/view/take_until.hpp	`94.54% <0%> (ø)`	⬆️
include/seqan3/range/view/pairwise_combine.hpp	`99.02% <0%> (ø)`	⬆️
.../seqan3/alignment/matrix/detail/trace_iterator.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/io/stream/iterator.hpp	`100% <0%> (ø)`	⬆️
include/seqan3/core/detail/strong_type.hpp	`100% <0%> (ø)`	⬆️
... and 5 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 96e1db6...ec619e2. Read the comment docs.

include/seqan3/range/view/async_buffer.hpp

rrahn · 2019-08-16T07:54:37Z

include/seqan3/range/view/async_buffer.hpp

+    class async_buffer_iterator
+    {
+        //!\brief The sentinel type to compare to.
+        using sentinel_type      = std::ranges::default_sentinel_t;


indentation

include/seqan3/range/view/async_buffer.hpp

rrahn · 2019-08-16T10:21:09Z

include/seqan3/range/view/async_buffer.hpp

+
+        while ((it != e) && (!stop_producer.load()))// TODO likely
+        {
+            auto tmp = std::move(*it);


The entire function can be reduced to:

void produce() { for (auto it = begin(urange); it != end(urange); ++it) if (buffer.wait_push(std::iter_move(it)) == queue_op_status::closed) break; buffer.close(); }

Then you can directly initialise it as a lambda function in the thread intialisation. But this is not important to me.

My original implementation was very close to this. I changed it after I got dead-locks, but looking at the code right now, I am not sure why exactly (I was under the false impression that try_push would report queue_op_status::full instead queue_op_status::closed if the queue is both full and closed).
I will investigate this again.

test/snippet/range/view/async_buffer.cpp

rrahn · 2019-08-16T12:23:45Z

test/unit/range/view/view_async_buffer_test.cpp

+using test_type = ::testing::Types<iterator_type>;
+INSTANTIATE_TYPED_TEST_CASE_P(iterator_fixture, iterator_fixture, test_type);
+
+


double new line

test/unit/range/view/view_async_buffer_test.cpp

rrahn

most of the minor comments where annotated but actually not fixed. Please check this again.
And we need to discuss another design if you really intend to have consumers not eating up the entire queue. I have layout one idea, but let's discuss this tomorrow with a fresh mind.

rrahn · 2019-08-19T21:03:13Z

include/seqan3/range/view/async_buffer.hpp

+ * Typically this adaptor is used when you want to consume the entire underlying range; there is no way to stop
+ * this view from buffering elements before the end of the underlying range is reached other than destructing it.
+ * If it is destructed before the end of the underlying range is reached, it will stop consuming elements, however,
+ * those elements that are currently in it's buffer are destructed with it.


😱 No this is a very bad design. If you want to allow that consumers are not consuming everything you need a different signaling mechanism, i.e. consumers signal that they are done reading. The simplest way to do it is by using a barrier then. In that case you need to specify in the construction of the view how many consumers you are expecting and the destructor needs to wait for the consumers to signal they are done (arrived at the barrier managed by the view). Then you can safely destruct the queue even if it is not empy after closing since you are sure that no one is reading from it anymore. I need to think about how you can make it work nicely with the iterator.

test/snippet/range/view/async_buffer.cpp

test/unit/range/view/view_async_buffer_test.cpp

h-2 · 2019-08-21T15:25:55Z

I have adapted everything according to our discussion.

most of the minor comments where annotated but actually not fixed. Please check this again.

Please review your comments and mark resolved issues as such so that I know which ones you are referring to. (Some things have been changed outside the immediate context so it may appear as there was no change when there was).

rrahn · 2019-08-23T10:21:11Z

include/seqan3/range/view/async_buffer.hpp

+    class async_buffer_iterator
+    {
+        //!\brief The sentinel type to compare to.
+        using sentinel_type      = std::ranges::default_sentinel_t;


rrahn · 2019-08-23T10:22:00Z

include/seqan3/range/view/async_buffer.hpp

+
+            detail::spin_delay delay{};
+
+            assert (buffer_ptr != nullptr);


include/seqan3/range/view/async_buffer.hpp

rrahn · 2019-08-23T10:43:16Z

include/seqan3/range/view/async_buffer.hpp

+ * the underlying range will be invalidated! For underlying ranges that are single-pass, this makes no difference, but
+ * it might be unexpected for multi-pass ranges (std::ranges::ForwardRange).
+ *
+ * Typically this adaptor is used when you want to consume the entire underlying range. While destructing


Suggested change

* Typically this adaptor is used when you want to consume the entire underlying range. While destructing

* Typically this adaptor is used when you want to consume the entire underlying range. Destructing

rrahn · 2019-08-23T10:47:20Z

include/seqan3/range/view/async_buffer.hpp

+ *
+ * Typically this adaptor is used when you want to consume the entire underlying range. While destructing
+ * this view before all elements have been read will also stop the thread that moves object from the underlying
+ * range, it is difficult to assess which elements in the underlying range might still be valid.


Suggested change

* range, it is difficult to assess which elements in the underlying range might still be valid.

* range.

rrahn · 2019-08-23T10:48:10Z

include/seqan3/range/view/async_buffer.hpp

+ * Typically this adaptor is used when you want to consume the entire underlying range. While destructing
+ * this view before all elements have been read will also stop the thread that moves object from the underlying
+ * range, it is difficult to assess which elements in the underlying range might still be valid.
+ * **In general, assume that it is not safe to access the underlying range separately once it has been passed


Suggested change

* **In general, assume that it is not safe to access the underlying range separately once it has been passed

* **In general, safe access to the underlying range in any other context is not guaranteed and once it has been passed

rrahn · 2019-08-23T10:48:51Z

include/seqan3/range/view/async_buffer.hpp

+ *
+ * Note that in addition to the buffer of the view, every iterator has its own one-element-buffer. Dereferencing
+ * the iterator returns a reference to the element in the buffer, usually you will want to move this element out
+ * of the buffer with std::move. Incrementing the iterator refills the buffer from the queue inside the


Suggested change

* of the buffer with std::move. Incrementing the iterator refills the buffer from the queue inside the

* of the buffer with std::move or std::iter_move. Incrementing the iterator refills the buffer from the queue inside the

test/snippet/range/view/async_buffer.cpp

rrahn · 2019-08-23T10:52:03Z

test/unit/range/view/view_async_buffer_test.cpp

+using test_type = ::testing::Types<iterator_type>;
+INSTANTIATE_TYPED_TEST_CASE_P(iterator_fixture, iterator_fixture, test_type);
+
+


test/unit/range/view/view_async_buffer_test.cpp

h-2 · 2019-08-26T17:06:54Z

Combinability is broken, don't merge this yet.

h-2 · 2019-08-29T13:18:37Z

I have addressed the last issues, I think. I even moved the iterator definition out 🤸‍♂️

h-2 · 2019-08-29T16:12:40Z

Fixed the test failure.

rrahn

some minor questions left

include/seqan3/range/detail/inherited_iterator_base.hpp

rrahn · 2019-08-30T12:34:35Z

include/seqan3/range/view/async_buffer.hpp

+    //!\brief Post-increment.
+    void operator++(int) noexcept
+    {
+        ++(*this);


so this iterator becomes no Cpp17Iterator. In case this matters.

It's not required: https://en.cppreference.com/w/cpp/named_req/InputIterator

value_type x = *i;
++i;
return x;

*i++ -> convertible to value_type

And further down:

typename std::common_reference_t<decltype(*i++)&&, typename std::readable_traits::value_type&>;

You are correct, but we wouldn't be able to satisfy "equivalent expression" in any case, right?

so we leave it as no CPP17Iterator. You might want to add then a sentence to the view description that the returned iterator does not model Cpp17InputIterator. After this we can merge it.

rrahn

add comment about not modeling Cpp17InputIterator to the view documentation.

rrahn

💅 only minor stuff

rrahn · 2019-09-02T22:31:28Z

test/unit/range/view/view_async_input_buffer_test.cpp

+         * And we want it to happen to make sure we don't dead-lock on it.
+         */
+        std::this_thread::sleep_for(std::chrono::milliseconds(100));
+


rrahn · 2019-09-03T22:14:51Z

CI is broken

rrahn

one more test, than I am happy 😏

rrahn · 2019-09-04T11:40:49Z

test/unit/range/view/view_async_input_buffer_test.cpp

+    EXPECT_FALSE(std::ranges::SizedRange<decltype(v1)>);
+    EXPECT_FALSE(ConstIterableRange<decltype(v1)>);
+    EXPECT_TRUE(std::ranges::View<decltype(v1)>);
+}


maybe add one more test with an empty range.

h-2 · 2019-09-05T15:21:39Z

Jenkins failure unrelated.

h-2 requested a review from eseiler August 8, 2019 16:58

eseiler requested changes Aug 9, 2019

View reviewed changes

eseiler reviewed Aug 9, 2019

View reviewed changes

test/unit/range/view/view_async_buffer_test.cpp Outdated Show resolved Hide resolved

eseiler reviewed Aug 9, 2019

View reviewed changes

include/seqan3/range/view/async_buffer.hpp Outdated Show resolved Hide resolved

h-2 force-pushed the view/async_buffer branch from c2cdbc5 to ffb0f7f Compare August 12, 2019 13:14

h-2 requested a review from eseiler August 12, 2019 13:14

eseiler requested changes Aug 12, 2019

View reviewed changes

h-2 force-pushed the view/async_buffer branch from ffb0f7f to 3175ead Compare August 14, 2019 11:59

h-2 requested a review from eseiler August 14, 2019 11:59

eseiler approved these changes Aug 14, 2019

View reviewed changes

h-2 requested a review from rrahn August 14, 2019 13:20

rrahn requested changes Aug 16, 2019

View reviewed changes

h-2 force-pushed the view/async_buffer branch from 3175ead to f940dbc Compare August 19, 2019 10:18

h-2 requested a review from rrahn August 19, 2019 10:18

h-2 force-pushed the view/async_buffer branch from f940dbc to 94af50b Compare August 19, 2019 11:50

rrahn requested changes Aug 19, 2019

View reviewed changes

h-2 force-pushed the view/async_buffer branch 2 times, most recently from 5f031e5 to 94b3e60 Compare August 21, 2019 15:24

h-2 requested a review from rrahn August 22, 2019 16:52

rrahn requested changes Aug 23, 2019

View reviewed changes

h-2 force-pushed the view/async_buffer branch from 649bc38 to aaa2ffa Compare August 29, 2019 13:17

h-2 requested a review from rrahn August 29, 2019 13:17

h-2 force-pushed the view/async_buffer branch 2 times, most recently from 06e93f2 to 414a66f Compare August 29, 2019 16:12

rrahn requested changes Aug 30, 2019

View reviewed changes

h-2 requested a review from rrahn September 2, 2019 12:11

rrahn requested changes Sep 2, 2019

View reviewed changes

h-2 force-pushed the view/async_buffer branch from 414a66f to 07b9122 Compare September 2, 2019 13:46

h-2 requested a review from rrahn September 2, 2019 13:46

rrahn requested changes Sep 2, 2019

View reviewed changes

[FEATURE] view::async_input_buffer

e78930d

h-2 force-pushed the view/async_buffer branch from 07b9122 to e78930d Compare September 3, 2019 12:15

h-2 requested a review from rrahn September 3, 2019 12:15

Merge branch 'master' into view/async_buffer

b443c61

rrahn requested changes Sep 4, 2019

View reviewed changes

fixes

ec619e2

h-2 merged commit c376e88 into seqan:master Sep 5, 2019

h-2 mentioned this pull request Sep 12, 2019

Async file I/O #1098

Closed

		using test_type = ::testing::Types<iterator_type>;
		INSTANTIATE_TYPED_TEST_CASE_P(iterator_fixture, iterator_fixture, test_type);

	* Typically this adaptor is used when you want to consume the entire underlying range. While destructing
	* Typically this adaptor is used when you want to consume the entire underlying range. Destructing

	* range, it is difficult to assess which elements in the underlying range might still be valid.
	* range.

	* **In general, assume that it is not safe to access the underlying range separately once it has been passed
	* **In general, safe access to the underlying range in any other context is not guaranteed and once it has been passed

	* of the buffer with std::move. Incrementing the iterator refills the buffer from the queue inside the
	* of the buffer with std::move or std::iter_move. Incrementing the iterator refills the buffer from the queue inside the

[FEATURE] view::async_buffer #1205

[FEATURE] view::async_buffer #1205

Conversation

h-2 commented Aug 8, 2019 • edited Loading

h-2 commented Aug 9, 2019

codecov bot commented Aug 12, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 commented Aug 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 commented Aug 26, 2019

h-2 commented Aug 29, 2019

h-2 commented Aug 29, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rrahn commented Sep 3, 2019

rrahn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h-2 commented Sep 5, 2019

h-2 commented Aug 8, 2019 •

edited

Loading

codecov bot commented Aug 12, 2019 •

edited

Loading