Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/15105 )
Change subject: WIP: Asynchronous code generation ...................................................................... Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/codegen/codegen-fn-ptr.h File be/src/codegen/codegen-fn-ptr.h: http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/codegen/codegen-fn-ptr.h@30 PS1, Line 30: rn ptr_.load(mem_order_); > Yeah I think we should either just hardcode the memory order as a static co Yes, probably the best solution is a constexpr memory order, I just wasn't sure in the beginning and thought we may want to test two different versions in the same build. What do you think would be the best memory order? Do you think relaxed should be enough? http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/codegen/codegen-fn-ptr.h@45 PS1, Line 45: inline CodegenFnPtrBase::~CodegenFnPtrBase() = default; > Would it make sense to make the function pointer type a template parameter I don't think we can do it type-safely because we store pointers to instances in containers where the instances have different underlying function pointers, for example in LlvmCodegen. But I have implemented a version where there is a general base class and the templated classes with concrete function types are subclasses of the base class. This way the function type is included in the type name of the atomic function pointers (good for code clarity) and the reinterpret casts can be collected to one place in the majority of the cases (however not all, for example 'codegend_fn_map_' in hdfs-scan-node.h still has to use void* as the function type because there are different kinds of functions in the collection). http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/exec/hdfs-sequence-scanner.cc File be/src/exec/hdfs-sequence-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/exec/hdfs-sequence-scanner.cc@310 PS1, Line 310: int tuples_returned = WriteAlignedTuplesCodegenOrInterpret(row_batch->tuple_data_pool(), > Maybe remove this comment? Not sure it is that informative. Done http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/exec/hdfs-text-scanner.cc File be/src/exec/hdfs-text-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/exec/hdfs-text-scanner.cc@869 PS1, Line 869: int tuples_returned = WriteAlignedTuplesCodegenOrInterpret(pool, row, fields, > Maybe remove this comment? Not sure it is that informative. Done http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/15105/1/be/src/runtime/runtime-state.h@200 PS1, Line 200: return !CodegenDisabledByQueryOption() && !CodegenDisabledByHint(); > nit: I think the member should be is_interpretable_ and the accessor method Done -- To view, visit http://gerrit.cloudera.org:8080/15105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7cbfa7c6734dcf03641629429057d6a4194aa6b Gerrit-Change-Number: 15105 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 04 Feb 2020 16:26:43 +0000 Gerrit-HasComments: Yes
