Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14973 )
Change subject: IMPALA-9274: cyclic barrier implementation ...................................................................... Patch Set 3: Code-Review+1 (3 comments) Had some questions but lgtm. 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 from different cycles. Isn't the goal of the CyclicBarrier to prohibit these overlappings? At first, Wait() will be invoked 'num_threads' times when j==0 in all threads (first cycle). Then, it will be invoked again 'num_threads' times when j==1 (second cycle), and so on. 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 'fn' will be copied 'num_threads' times but I guess we can expect it to be fairly small. Java's CyclicBarrier.await() returns an "arrival index" that can be used to execute a function after each cycle. But I'm OK with the current solution since it's more elegant and the overhead probably negligable. 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 L61 so it could run in parallel with the threads working on the next cycle? It can be beneficial if the work items are dynamic or have variable size and fn() is not trivial. On the other hand it's definitely more safe and simpler this way. -- 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: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 03 Jan 2020 13:57:21 +0000 Gerrit-HasComments: Yes
