Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/11650 )
Change subject: IMPALA-7272: Fix crash in StringMinMaxFilter ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/11650/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11650/2//COMMIT_MSG@26 PS2, Line 26: I have been unable to repro the actual crash despite trying a large : variety of different things. However, with additional logging added : its clear that the MemPool is being used concurrently, which is : incorrect. Can you reproduce the problem now with DFAKE_MUTEX ? http://gerrit.cloudera.org:8080/#/c/11650/2/be/CMakeLists.txt File be/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11650/2/be/CMakeLists.txt@234 PS2, Line 234: SET(CLANG_IR_CXX_FLAGS "${CLANG_IR_CXX_FLAGS}" "-DNDEBUG") I think this makes sense. I wonder what the historical context for not doing it in the first place. I checked the code on some older releases from more than 4 years ago and it was there all along. May be the codegen time was too slow back then to include all the debug code ? How much slow down (if any) did you notice in debug builds with this change ? http://gerrit.cloudera.org:8080/#/c/11650/2/be/src/runtime/mem-pool.h File be/src/runtime/mem-pool.h: http://gerrit.cloudera.org:8080/#/c/11650/2/be/src/runtime/mem-pool.h@135 PS2, Line 135: DFAKE_SCOPED_LOCK(mutex_); nit: Feel free to ignore but it seems easier to just add DFAKE_SCOPED_LOCK() directly in Allocate(). That said, the current code also looks fine as-is. -- To view, visit http://gerrit.cloudera.org:8080/11650 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I751cad7e6b75c9d95d7ad029bbd1e52fe09e8a29 Gerrit-Change-Number: 11650 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Tue, 16 Oct 2018 23:40:22 +0000 Gerrit-HasComments: Yes
