Todd Lipcon has posted comments on this change. Change subject: WIP: flatbuffers-based codegen ......................................................................
Patch Set 2: (3 comments) thanks for taking a look Vlad! http://gerrit.cloudera.org:8080/#/c/2903/2/src/kudu/codegen/precompiled.cc File src/kudu/codegen/precompiled.cc: Line 111: 0 > #if 0 ? this was is a hackathon-produced WIP so some messiness in here still needs a cleanup. You found one such messiness :) (this should probably just be an NDEBUG check) Line 142: for (auto dst_col_idx : *fb->default_cols()) { > Why no #pragma unroll here? Why #pragma unroll at all? Shouldn't LLVM figur ah, probably should be one. The #pragma unroll is because clang's heuristic is to not unroll loops with known trip counts if the trip count is more than 8. So for schemas with <8 columns, it'll unroll anyway, but with more, you have to tell it to do so. It's not always smart enough to see that by unrolling the loop it gets to eliminate a lot of constants. http://gerrit.cloudera.org:8080/#/c/2903/2/src/kudu/codegen/row_projector.cc File src/kudu/codegen/row_projector.cc: Line 286: // TODO: it seems like there's a bug here since we're using the pointer value : // of the read_default in the case of STRINGs, which could get reused. > Why would that be a bug? If it gets reused then the pointer is still valid I guess you're right - if it got reused for the same type in the same location then the code would still work. I was thinking somehow that we'd actually inlined the _value_ of the default, which isn't the case of course (and if we had, then we wouldnt' care if the pointer changed) -- To view, visit http://gerrit.cloudera.org:8080/2903 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I583ef1edad379bb13b34362f121263de28c8334c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Anonymous Coward #174 Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
