Tim Armstrong has posted comments on this change. Change subject: IMPALA-4026: Implement double-buffering for BlockingQueue. ......................................................................
Patch Set 1: (5 comments) The new algorithm makes sense to me. We could probably get fancier but this seems like a nice balance of simplicity and performance. Mostly high-level comments so far http://gerrit.cloudera.org:8080/#/c/4350/1//COMMIT_MSG Commit Message: Line 29: BlockingQueue to using POSIX pthread primitives instead of boost This seems strange. I looked at ./boost_1_57_0/boost/thread/pthread/mutex.hpp in the toolchain and the boost mutex is just a thin wrapper around pthread_mutex. Line 45: | TARGETED-PERF(_300) | primitive_conjunct_ordering_1 | parquet / none / none | 10.72 | 9.84 | +8.95% | 2.57% | 0.53% | 1 | 3 | Any idea why these regressed? Might be good to understand if there's something we can do to fix the regression or improve performance further. http://gerrit.cloudera.org:8080/#/c/4350/1/be/src/util/blocking-queue.h File be/src/util/blocking-queue.h: Line 65: pthread_mutex_lock(&read_lock_); I think it would be good to stick with the unique_lock interface - I find it harder to convince myself that everything is unlocked on all code paths otherwise. I'm not sure why the boost mutex is slower, but maybe it's because it's checking the error codes and throwing an exception. If this is the problem, I think we should consider writing our own wrapper for pthread_mutex without the exceptions (maybe just DCHECKs?). The required lockable interface is pretty simple - SpinLock already implements it. PS1, Line 186: shutted down. shut down. Line 201: /// Guards against concurrent access to 'write_list_'. Maybe document lock order here too? -- To view, visit http://gerrit.cloudera.org:8080/4350 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib9f4cf351455efefb0f3bb791cf9bc82d1421d54 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
