Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14973 )
Change subject: IMPALA-9274: cyclic barrier implementation ...................................................................... Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier-test.cc File be/src/util/cyclic-barrier-test.cc: http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier-test.cc@69 PS3, Line 69: calls to Wait() from different cycles can overlap. > I might be missing stg but I don't see how can the Wait() calls overlap fro I added some explanation to the comment - the overlap is from different threads being in different iterations at the same time. Without the cycle_num_ logic there are various races possible where threads get stuck. http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier.h File be/src/util/cyclic-barrier.h: http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier.h@40 PS3, Line 40: F fn Nice catch, I changed it to passing by reference. I think lambda objects should generally be pretty lightweight to construct (since we're passing in lambda type and don't need to convert it to a std::function). > Java's CyclicBarrier.await() returns an "arrival index" that can be used to > execute a function after each cycle. The other big different is that the function runs before unblocking all the other threads, which is convenient for the join algorithm where we want to run some code serially before the next parallel phase. http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier.h@56 PS3, Line 56: fn(); > Does it need to run as part of the cycle? Would it make sense to move it to This was deliberate - my initial use case is to execute the serial part of the spilling algorithm. I added a bit more explanation to the function comment. I think we could generalise later on if needed. http://gerrit.cloudera.org:8080/#/c/14973/3/be/src/util/cyclic-barrier.h@71 PS3, Line 71: // The number of threads partipating in synchronization. > Nit: participating Done -- To view, visit http://gerrit.cloudera.org:8080/14973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5c8cc6b3023d2dc03c8116d462440c5e25c3bb0 Gerrit-Change-Number: 14973 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sun, 05 Jan 2020 01:17:40 +0000 Gerrit-HasComments: Yes
