Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20692 )
Change subject: IMPALA-11542: Implement pre-allocation in Section Memory Manager ...................................................................... Patch Set 4: (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: CodeAlign Do you know what is the role of alignment here and what values can we expect? My impression is that if alignment actually modifies requiredPageSize then the logic below won't ensure proper alignment. E.g. if page size is 4096, alignment is 8192 and we allocate 5000 byte memory, then if the allocation is only page size aligned, then we won't be able to cram the alignment offset + payload to the block http://gerrit.cloudera.org:8080/#/c/20692/4/be/src/codegen/mcjit-mem-mgr.cc@143 PS4, Line 143: DCHECK(!needsToReserveAllocationSpace()) : << "All memory should be pre-allocated: " << Size << "/" << Alignment; What comes after this seems essentially dead code, as needsToReserveAllocationSpace() is set to true. Or you plan to set it to true only in case we are compiling for ARM64? -- 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: 4 Gerrit-Owner: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Yida Wu <wydbaggio...@gmail.com> Gerrit-Comment-Date: Fri, 10 Nov 2023 12:03:33 +0000 Gerrit-HasComments: Yes
