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 4: (16 comments) http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20733/3//COMMIT_MSG@28 PS3, Line 28: > Do you mean run the time running all the tpch queries for once? Did a test, Doesn't the benchmarking framework print a grand total? If not, we can let it be like this. http://gerrit.cloudera.org:8080/#/c/20733/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20733/4//COMMIT_MSG@17 PS4, Line 17: (execution engine) Could you clarify the relationship of the module and exec engine here? I.e. one execution engine corresponds to exactly one module - is it correct? http://gerrit.cloudera.org:8080/#/c/20733/4//COMMIT_MSG@18 PS4, Line 18: pre-compiled Why are they pre-compiled? Aren't they compiled already? Or you mean they are not compiled to machine code? Then "partially compiled" or "optimized" may be better. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-cache.h File be/src/codegen/llvm-codegen-cache.h: http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-cache.h@164 PS4, Line 164: engine_cache_ Thinking about this, wouldn't 'cached_engine...' express the meaning better? This is not a cache of multiple engines but a cached instance of an engine, or even better, its functions. Maybe 'cached_module...'? This applies also to the other variables named similarly in this file. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-cache.cc File be/src/codegen/llvm-codegen-cache.cc: http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-cache.cc@155 PS4, Line 155: ! I think removing the negation and inverting the branches is better, one less negation to process mentally. See also L179. 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@39 PS4, Line 39: getMemBufferCopy Is a copy necessary here? Can't we move the buffer somehow? http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-object-cache.cc@42 PS4, Line 42: LOG(WARNING) << "Inserting a different module in CodeGenObjectCache " << this Shouldn't we add a DCHECK here? If this happens it is a bug. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-object-cache.cc@57 PS4, Line 57: return llvm::MemoryBuffer::getMemBufferCopy( Will it always copy the buffer? Is there a way to access it without copying? http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-object-cache.cc@60 PS4, Line 60: LOG(WARNING) << "Object for module id " << module_id << " can't be loaded from cache." Shouldn't we add a DCHECK here? If this happens it is a bug. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen-object-cache.cc@63 PS4, Line 63: return nullptr; Can this function be called when 'key_' is empty? Isn't that a bug? http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen.h File be/src/codegen/llvm-codegen.h: http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen.h@330 PS4, Line 330: engine_cache See comment on llvm-codegen-cache.h:164. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen.h@925 PS4, Line 925: /// Object cache for engine. Do I understand it correctly that this is for writing to the cache (storing the module we compiled) and 'engine_cache_cached_' is for reading (when we retrieve a module from the cache)? It could be clarified in the comment(s). http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen.h@929 PS4, Line 929: /// Not null only when there is a cache hit. We could extend the comment with what is written on llvm-codegen.cc:1499, i.e. that we need the shared_ptr so that the lifetime of this object doesn't end even if it is evicted from the cache while we're using it. http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/20733/4/be/src/codegen/llvm-codegen.cc@1192 PS4, Line 1192: entire execution engine We no longer cache the engine. http://gerrit.cloudera.org:8080/#/c/20733/4/tests/custom_cluster/test_codegen_cache.py File tests/custom_cluster/test_codegen_cache.py: http://gerrit.cloudera.org:8080/#/c/20733/4/tests/custom_cluster/test_codegen_cache.py@169 PS4, Line 169: When an 'llvm::ExecutionEngine', which is part of the cache entry, is destroyed, it The situation described in this paragraph is no longer true after this change. We should write that this is how it used to work and also describe how it has changed after this patch. We should mention the Jira number of this patch. http://gerrit.cloudera.org:8080/#/c/20733/4/tests/custom_cluster/test_codegen_cache.py@228 PS4, Line 228: fragment 'fragments_ran' (plural) would be better. Applies also to L239. -- 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: 4 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: Tue, 05 Dec 2023 10:27:33 +0000 Gerrit-HasComments: Yes
