Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20692 )
Change subject: IMPALA-11542: Implement pre-allocation in LLVM memory manager ...................................................................... Patch Set 9: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/20692/4/be/src/codegen/mcjit-mem-mgr.cc File be/src/codegen/mcjit-mem-mgr.cc: http://gerrit.cloudera.org:8080/#/c/20692/4/be/src/codegen/mcjit-mem-mgr.cc@49 PS4, Line 49: > Alignment is probably smaller than page size, and must be a power of 2. I b I did check the comments, but I didn't find anything that would rule out the PageSize< Size < Alignment case. I think that it would be better to DCHECK for this, as the allocation wouldn't be large enough in the problematic case. Things should be ok if both PageSize and Alignment are power of 2 and (size >= Alignment or PageSize => Alignment ) http://gerrit.cloudera.org:8080/#/c/20692/4/be/src/codegen/mcjit-mem-mgr.cc@93 PS4, Line 93: : : : : : : : : : : : : > I've split commits up so it should be much clearer. Thanks for splitting the commit! I still think that it would be useful to add a few comments about what is changed and what not, e.g. a file level comment about everything being copied and a function level comment about being new/modified in Impala -- To view, visit http://gerrit.cloudera.org:8080/20692 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9f224edcdbdcb05fce663c18b4a5f03c8e985675 Gerrit-Change-Number: 20692 Gerrit-PatchSet: 9 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Mon, 13 Nov 2023 14:21:24 +0000 Gerrit-HasComments: Yes
