Use double-buffering to fetch the records from the one during iteration #60

nemanja-boric-sociomantic · 2018-02-16T15:48:07Z

The previous approach where the client would ask for the batch from the
node was prone to the latency problems - if the time to prepare and send
batch is non-negligible the performance of the client application would
suffer. This patch improves the situation and decreases the latency in
the following way:

As soon as client get the new batch from the node, it will request another one
When the new batch arrives the client will store it in the back
buffer, ready for processing as soon as it finish with the previous one
After processing the previous batch and the new batch already arrived,
the client will just swap the buffers, reading from the new one.

david-eckardt-sociomantic · 2018-02-16T15:51:17Z

src/dlsproto/client/request/internal/GetRange.d

+            private void[]*[2] batch_buffers;
+
+            /// Index of the batch we're currently iterating over
+            private int current_buffer;


I would use bool...

I would still prefer int though, because it doesn't restrict the number of the buffers artificially to two (it's easy to generalize this to N buffers this way). For the same reason I would stick with modulo arithmetic.

As an alternative you could use void[]* front_buffer, back_buffer and swap them in the swap method. This would avoid the need for current_buffer and the buffer getter methods.

OK, that's more neat. I'll take that suggestion 👍

david-eckardt-sociomantic · 2018-02-16T15:52:57Z

src/dlsproto/client/request/internal/GetRange.d

+            /// Swaps the front and back buffer
+            private void swap ()
+            {
+                this.current_buffer = (this.current_buffer + 1) % 2;


this.current_buffer = !this.current_buffer

?

david-eckardt-sociomantic · 2018-02-16T15:53:49Z

src/dlsproto/client/request/internal/GetRange.d

+            private int current_buffer;
+
+            /// Initializes the double buffer
+            void init (void[]* delegate() getVoidBuffer)


lazy void[]* if you want to be cool ;)

Hmm, I think the way it is consistent with rest of the swarm - we're passing acquires as delegates.

nemanja-boric-sociomantic · 2018-02-16T16:03:08Z

Adapted to @david-eckardt-sociomantic 's proposal with fields.

david-eckardt-sociomantic · 2018-02-16T16:05:21Z

src/dlsproto/client/request/internal/GetRange.d

+            {
+                auto tmp = this.front_buffer;
+                this.front_buffer = this.back_buffer;
+                this.back_buffer = tmp;


typeid(void[]*).swap(&this.front_buffer, &this.back_buffer)

(just kidding)

david-eckardt-sociomantic · 2018-02-16T16:14:06Z

src/dlsproto/client/request/internal/GetRange.d

+            private void[]* back_buffer;
+
+            /// Initializes the double buffer
+            void init (void[]* delegate() getVoidBuffer)


nemanja-boric-sociomantic · 2018-02-16T16:15:00Z

Updated with private init and with disallowing the client to stop without going through the back buffer.

gavin-norman-sociomantic · 2018-02-16T16:18:58Z

src/dlsproto/client/request/internal/GetRange.d

+            /// Acquired buffers to store batches of records. Reader always
+            /// appends to the back one and the RecordStream always uses the
+            /// front one.
+            private void[]* front_buffer;


You could even name them reader_buffer and record_stream_buffer, to make it super clear which is which.

Right, or just output and input. (The _buffer suffix isn't needed.)

david-eckardt-sociomantic · 2018-02-16T16:21:00Z

src/dlsproto/client/request/internal/GetRange.d

+            // Grab the back buffer and move it to the front
+            this.buffers.swap();
+
+            return !this.stopped || this.buffers.front_buffer.length;


Tiny adjustment: (*this.buffers.front_buffer).length, for consistency.

gavin-norman-sociomantic · 2018-02-16T16:24:48Z

A higher level idea: would it be possible to completely encapsulate the buffer handling, so that the reader and record stream fibers don't need to know anything about it? They would just push and pop records to/from the buffer.

nemanja-boric-sociomantic · 2018-02-16T16:25:43Z

Note that the Reader already doesn't know anything about the buffer. I'll see if I can encapsulate it so the RecordStream doesn't know anything.

nemanja-boric-sociomantic · 2018-02-16T18:17:01Z

Updated with encapsulation of empty() method. I've tried making it higher-level, but I'm not happy with the end results. The problem is that now the RecordStream doesn't know anything about buffers, it's not just the point of the swapping, but it also doesn't know when it should send Continue signal to the node. This requires delegating the send continue signal from the RecordStream to the buffer class, but that then makes this Buffer not just maintaining two buffers & swap, but to know that the data needs to be requested and that it doesn't come automatically.

I think right now the complexity is quite low, I wouldn't try going further than this, but I'm open to comments.

gavin-norman-sociomantic · 2018-02-19T09:09:08Z

Updated with encapsulation of empty() method. I've tried making it higher-level, but I'm not happy with the end results. The problem is that now the RecordStream doesn't know anything about buffers, it's not just the point of the swapping, but it also doesn't know when it should send Continue signal to the node. This requires delegating the send continue signal from the RecordStream to the buffer class, but that then makes this Buffer not just maintaining two buffers & swap, but to know that the data needs to be requested and that it doesn't come automatically.

Fair enough. I think it probably could be done in a nice way, but it's not incredibly important, right now.

gavin-norman-sociomantic · 2018-02-19T08:40:28Z

src/dlsproto/client/request/internal/GetRange.d

+            /// ditto
+            private void[]* input;
+
+            /// Initializes the double buffer


Hey! I just noticed you're trying to sneak in a new method doc format! Cheeky ;)

How would you ever set the precedent without sneaking it in? :-)

gavin-norman-sociomantic · 2018-02-19T08:40:58Z

src/dlsproto/client/request/internal/GetRange.d

+            /// front one.
+            private void[]* output;
+
+            /// ditto


It'd be clearer for each buffer to have its own comment.

gavin-norman-sociomantic · 2018-02-19T08:53:53Z

src/dlsproto/client/request/internal/GetRange.d


        **********************************************************************/

        private bool waitForRecords ()
        {
-            this.suspendFiber(FiberSuspended.WaitingForRecords);
-            return !this.stopped;
+            // In case we already have the batch, don't wait for it


Would make more sense reversed: "Wait for the next batch, unless we already have one.".

gavin-norman-sociomantic · 2018-02-19T09:10:04Z

src/dlsproto/client/request/internal/GetRange.d

-
-            if (!(*this.batch_buffer).length)
-                enableStomping(*this.batch_buffer);
+            if (!(*batch_buffer).length)


Would seem sensible (and simple) to encapsulate this in some kind of push method.

The previous approach where the client would ask for the batch from the node was prone to the latency problems - if the time to prepare and send batch is non-negligible the performance of the client application would suffer. This patch improves the situation and decreases the latency in the following way: - As soon as client get the new batch from the node, it will request another one - When the new batch arrives the client will store it in the back buffer, ready for processing as soon as it finish with the previous one - After processing the previous batch and the new batch already arrived, the client will just swap the buffers, reading from the new one.

nemanja-boric-sociomantic · 2018-02-19T16:03:11Z

Updated with the addressed comments by @gavin-norman-sociomantic .

nemanja-boric-sociomantic added the type-feature label Feb 16, 2018

nemanja-boric-sociomantic added this to the neo-beta-1 milestone Feb 16, 2018

nemanja-boric-sociomantic requested review from gavin-norman-sociomantic and david-eckardt-sociomantic February 16, 2018 15:48

nemanja-boric-sociomantic force-pushed the better-way branch from 51a923d to 6dc1f9f Compare February 16, 2018 15:49

david-eckardt-sociomantic reviewed Feb 16, 2018

View reviewed changes

nemanja-boric-sociomantic force-pushed the better-way branch from 6dc1f9f to e447603 Compare February 16, 2018 16:02

david-eckardt-sociomantic reviewed Feb 16, 2018

View reviewed changes

nemanja-boric-sociomantic force-pushed the better-way branch from e447603 to 754910f Compare February 16, 2018 16:14

nemanja-boric-sociomantic force-pushed the better-way branch from 754910f to 618cdbb Compare February 16, 2018 16:17

gavin-norman-sociomantic reviewed Feb 16, 2018

View reviewed changes

david-eckardt-sociomantic reviewed Feb 16, 2018

View reviewed changes

nemanja-boric-sociomantic force-pushed the better-way branch from 618cdbb to 266b211 Compare February 16, 2018 18:13

david-eckardt-sociomantic approved these changes Feb 16, 2018

View reviewed changes

gavin-norman-sociomantic reviewed Feb 19, 2018

View reviewed changes

nemanja-boric-sociomantic force-pushed the better-way branch from 266b211 to bc0fb50 Compare February 19, 2018 16:02

gavin-norman-sociomantic approved these changes Feb 19, 2018

View reviewed changes

nemanja-boric-sociomantic merged commit 11dba97 into sociomantic-tsunami:neo Feb 19, 2018

nemanja-boric-sociomantic deleted the better-way branch February 19, 2018 16:39

nemanja-boric-sociomantic mentioned this pull request Feb 22, 2018

DoubleBuffering implementation in GetRange sometimes hangs #62

Closed

nemanja-boric-sociomantic modified the milestones: neo-beta-1, neo-alpha-4 Feb 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use double-buffering to fetch the records from the one during iteration #60

Use double-buffering to fetch the records from the one during iteration #60

nemanja-boric-sociomantic commented Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

nemanja-boric-sociomantic Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

nemanja-boric-sociomantic Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

nemanja-boric-sociomantic Feb 16, 2018

nemanja-boric-sociomantic commented Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

nemanja-boric-sociomantic commented Feb 16, 2018

gavin-norman-sociomantic Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

david-eckardt-sociomantic Feb 16, 2018

gavin-norman-sociomantic commented Feb 16, 2018

nemanja-boric-sociomantic commented Feb 16, 2018

nemanja-boric-sociomantic commented Feb 16, 2018

gavin-norman-sociomantic commented Feb 19, 2018

gavin-norman-sociomantic Feb 19, 2018

nemanja-boric-sociomantic Feb 19, 2018

gavin-norman-sociomantic Feb 19, 2018

gavin-norman-sociomantic Feb 19, 2018

gavin-norman-sociomantic Feb 19, 2018

nemanja-boric-sociomantic commented Feb 19, 2018

Use double-buffering to fetch the records from the one during iteration #60

Use double-buffering to fetch the records from the one during iteration #60

Conversation

nemanja-boric-sociomantic commented Feb 16, 2018

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

nemanja-boric-sociomantic commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemanja-boric-sociomantic commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavin-norman-sociomantic commented Feb 16, 2018

nemanja-boric-sociomantic commented Feb 16, 2018

nemanja-boric-sociomantic commented Feb 16, 2018

gavin-norman-sociomantic commented Feb 19, 2018

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

nemanja-boric-sociomantic commented Feb 19, 2018