Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20733 )
Change subject: IMPALA-11805: Use llvm ObjectCache for codegen caching ...................................................................... Patch Set 7: (10 comments) Thanks Yida! http://gerrit.cloudera.org:8080/#/c/20733/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20733/7//COMMIT_MSG@17 PS7, Line 17: after Could add "and" before "after". http://gerrit.cloudera.org:8080/#/c/20733/7//COMMIT_MSG@19 PS7, Line 19: funtions Nit: funCtions http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-cache-test.cc File be/src/codegen/llvm-codegen-cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-cache-test.cc@37 PS7, Line 37: // The object cache size for an engine containing a single compiled Could include that it is in bytes. http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-cache-test.cc@41 PS7, Line 41: _LARGE Are there tests with small (non-large) capacity? Otherwise it doesn't need to be included in the name. http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-cache-test.cc@50 PS7, Line 50: // Using single shard makes the logic of scenarios simple for capacity and : // eviction-related behavior. : FLAGS_cache_force_single_shard = true; Duplicate lines from L46. http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-cache-test.cc@114 PS7, Line 114: shared_ptr<CodeGenObjectCache> CreateObjCache( I don't think the arguments should have default values. There is only one place where this function is called without arguments, it could explicitly pass (false, nullptr). http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-cache-test.cc@271 PS7, Line 271: codegen This could also be 'codegen_echo'. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-object-cache.cc File be/src/codegen/llvm-codegen-object-cache.cc: http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-object-cache.cc@57 PS4, Line 57: // ID. If the ID doesn't match the one in the object cache, a warning is logged for > From the definition of the interface getObject(), "Returns a pointer to a n I haven't checked what MemoryBuffer objects are, but is it true that we've already copied the buffer into 'cached_obj_'? Or who owns it? Is it possible that the reference we give out will outlive this object? If so, we should put the buffer behind a shared pointer, if not, then we could safely give out references to it. I'm not familiar with how these objects can be used in LLVM but always copying the whole module buffer seems wasteful. http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-object-cache.cc File be/src/codegen/llvm-codegen-object-cache.cc: http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-object-cache.cc@39 PS7, Line 39: << ObjBuffer.getBuffer().size(); Some context could be provided, like "size: " and the unit (bytes?). Applies also to L62. http://gerrit.cloudera.org:8080/#/c/20733/7/be/src/codegen/llvm-codegen-object-cache.cc@56 PS7, Line 56: object cache Isn't it the "cached object" instead? See also on the next line. -- To view, visit http://gerrit.cloudera.org:8080/20733 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic3c1b46bb9018ed0320817141785a3bdc41fa677 Gerrit-Change-Number: 20733 Gerrit-PatchSet: 7 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Fri, 15 Dec 2023 12:23:10 +0000 Gerrit-HasComments: Yes
