[3/3] incubator-impala git commit: Remove caching of llvm StructType by TupleDescriptor::GetLlvmStruct().

[lv...@apache.org]

Remove caching of llvm StructType by TupleDescriptor::GetLlvmStruct().

This caching is causing problems when multiple threads are
involved in query startup, which is being implemented as
part of the move to a per-query exec rpc (IMPALA-2550).

Change-Id: I6f765048f36b9e596aeb4e4482d38b94f5ffba06
Reviewed-on: http://gerrit.cloudera.org:8080/6511
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/4c4235f4
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/4c4235f4
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/4c4235f4

Branch: refs/heads/master
Commit: 4c4235f4b23a6050d5a5efe9f358fca719e97c19
Parents: a7bd260
Author: Marcel Kornacker <ma...@cloudera.com>
Authored: Wed Mar 29 12:31:51 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Mar 31 03:26:53 2017 +0000

----------------------------------------------------------------------
 be/src/runtime/descriptors.cc | 82 +++++++++++++++++++-------------------
 be/src/runtime/descriptors.h  |  4 --
 2 files changed, 40 insertions(+), 46 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4c4235f4/be/src/runtime/descriptors.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/descriptors.cc b/be/src/runtime/descriptors.cc
index 5f71bf7..52219e6 100644
--- a/be/src/runtime/descriptors.cc
+++ b/be/src/runtime/descriptors.cc
@@ -35,7 +35,6 @@
 #include "common/names.h"
 
 using boost::algorithm::join;
-using namespace llvm;
 using namespace strings;
 
 // In 'thrift_partition', the location is stored in a compressed format that references
@@ -294,8 +293,7 @@ TupleDescriptor::TupleDescriptor(const TTupleDescriptor& tdesc)
     null_bytes_offset_(tdesc.byteSize - tdesc.numNullBytes),
     slots_(),
     has_varlen_slots_(false),
-    tuple_path_(tdesc.tuplePath),
-    llvm_struct_(NULL) {
+    tuple_path_(tdesc.tuplePath) {
 }
 
 void TupleDescriptor::AddSlot(SlotDescriptor* slot) {
@@ -595,8 +593,8 @@ void DescriptorTbl::GetTupleDescs(vector<TupleDescriptor*>* descs) const {
   }
 }
 
-Value* SlotDescriptor::CodegenIsNull(
-    LlvmCodeGen* codegen, LlvmBuilder* builder, Value* tuple) const {
+llvm::Value* SlotDescriptor::CodegenIsNull(
+    LlvmCodeGen* codegen, LlvmBuilder* builder, llvm::Value* tuple) const {
   return CodegenIsNull(codegen, builder, null_indicator_offset_, tuple);
 }
 
@@ -606,14 +604,14 @@ Value* SlotDescriptor::CodegenIsNull(
 //  %null_byte = load i8, i8* %null_byte_ptr
 //  %null_mask = and i8 %null_byte, 1
 //  %is_null = icmp ne i8 %null_mask, 0
-Value* SlotDescriptor::CodegenIsNull(LlvmCodeGen* codegen, LlvmBuilder* builder,
-    const NullIndicatorOffset& null_indicator_offset, Value* tuple) {
-  Value* null_byte =
+llvm::Value* SlotDescriptor::CodegenIsNull(LlvmCodeGen* codegen, LlvmBuilder* builder,
+    const NullIndicatorOffset& null_indicator_offset, llvm::Value* tuple) {
+  llvm::Value* null_byte =
       CodegenGetNullByte(codegen, builder, null_indicator_offset, tuple, NULL);
-  Constant* mask =
-      ConstantInt::get(codegen->tinyint_type(), null_indicator_offset.bit_mask);
-  Value* null_mask = builder->CreateAnd(null_byte, mask, "null_mask");
-  Constant* zero = ConstantInt::get(codegen->tinyint_type(), 0);
+  llvm::Constant* mask =
+      llvm::ConstantInt::get(codegen->tinyint_type(), null_indicator_offset.bit_mask);
+  llvm::Value* null_mask = builder->CreateAnd(null_byte, mask, "null_mask");
+  llvm::Constant* zero = llvm::ConstantInt::get(codegen->tinyint_type(), 0);
   return builder->CreateICmpNE(null_mask, zero, "is_null");
 }
 
@@ -627,18 +625,19 @@ Value* SlotDescriptor::CodegenIsNull(LlvmCodeGen* codegen, LlvmBuilder* builder,
 //  %null_bit_set = or i8 %null_bit_cleared, %null_bit
 //  store i8 %null_bit_set, i8* %null_byte_ptr3
 void SlotDescriptor::CodegenSetNullIndicator(
-    LlvmCodeGen* codegen, LlvmBuilder* builder, Value* tuple, Value* is_null) const {
+    LlvmCodeGen* codegen, LlvmBuilder* builder, llvm::Value* tuple, llvm::Value* is_null)
+    const {
   DCHECK_EQ(is_null->getType(), codegen->boolean_type());
-  Value* null_byte_ptr;
-  Value* null_byte =
+  llvm::Value* null_byte_ptr;
+  llvm::Value* null_byte =
       CodegenGetNullByte(codegen, builder, null_indicator_offset_, tuple, &null_byte_ptr);
-  Constant* mask =
-      ConstantInt::get(codegen->tinyint_type(), null_indicator_offset_.bit_mask);
-  Constant* not_mask =
-      ConstantInt::get(codegen->tinyint_type(), ~null_indicator_offset_.bit_mask);
+  llvm::Constant* mask =
+      llvm::ConstantInt::get(codegen->tinyint_type(), null_indicator_offset_.bit_mask);
+  llvm::Constant* not_mask =
+      llvm::ConstantInt::get(codegen->tinyint_type(), ~null_indicator_offset_.bit_mask);
 
-  ConstantInt* constant_is_null = dyn_cast<ConstantInt>(is_null);
-  Value* result = NULL;
+  llvm::ConstantInt* constant_is_null = llvm::dyn_cast<llvm::ConstantInt>(is_null);
+  llvm::Value* result = NULL;
   if (constant_is_null != NULL) {
     if (constant_is_null->isOne()) {
       result = builder->CreateOr(null_byte, mask, "null_bit_set");
@@ -649,37 +648,37 @@ void SlotDescriptor::CodegenSetNullIndicator(
   } else {
     // Avoid branching by computing the new byte as:
     // (null_byte & ~mask) | (-null & mask);
-    Value* byte_with_cleared_bit =
+    llvm::Value* byte_with_cleared_bit =
         builder->CreateAnd(null_byte, not_mask, "null_bit_cleared");
-    Value* sign_extended_null = builder->CreateSExt(is_null, codegen->tinyint_type());
-    Value* bit_only = builder->CreateAnd(sign_extended_null, mask, "null_bit");
+    llvm::Value* sign_extended_null =
+        builder->CreateSExt(is_null, codegen->tinyint_type());
+    llvm::Value* bit_only = builder->CreateAnd(sign_extended_null, mask, "null_bit");
     result = builder->CreateOr(byte_with_cleared_bit, bit_only, "null_bit_set");
   }
 
   builder->CreateStore(result, null_byte_ptr);
 }
 
-Value* SlotDescriptor::CodegenGetNullByte(LlvmCodeGen* codegen, LlvmBuilder* builder,
-    const NullIndicatorOffset& null_indicator_offset, Value* tuple,
-    Value** null_byte_ptr) {
-  Constant* byte_offset =
-      ConstantInt::get(codegen->int_type(), null_indicator_offset.byte_offset);
-  Value* tuple_bytes = builder->CreateBitCast(tuple, codegen->ptr_type());
-  Value* byte_ptr = builder->CreateInBoundsGEP(tuple_bytes, byte_offset, "null_byte_ptr");
+llvm::Value* SlotDescriptor::CodegenGetNullByte(
+    LlvmCodeGen* codegen, LlvmBuilder* builder,
+    const NullIndicatorOffset& null_indicator_offset, llvm::Value* tuple,
+    llvm::Value** null_byte_ptr) {
+  llvm::Constant* byte_offset =
+      llvm::ConstantInt::get(codegen->int_type(), null_indicator_offset.byte_offset);
+  llvm::Value* tuple_bytes = builder->CreateBitCast(tuple, codegen->ptr_type());
+  llvm::Value* byte_ptr =
+      builder->CreateInBoundsGEP(tuple_bytes, byte_offset, "null_byte_ptr");
   if (null_byte_ptr != NULL) *null_byte_ptr = byte_ptr;
   return builder->CreateLoad(byte_ptr, "null_byte");
 }
 
-StructType* TupleDescriptor::GetLlvmStruct(LlvmCodeGen* codegen) const {
-  // If we already generated the llvm type, just return it.
-  if (llvm_struct_ != NULL) return llvm_struct_;
-
+llvm::StructType* TupleDescriptor::GetLlvmStruct(LlvmCodeGen* codegen) const {
   // Sort slots in the order they will appear in LLVM struct.
   vector<SlotDescriptor*> sorted_slots(slots_.size());
   for (SlotDescriptor* slot: slots_) sorted_slots[slot->slot_idx_] = slot;
 
   // Add the slot types to the struct description.
-  vector<Type*> struct_fields;
+  vector<llvm::Type*> struct_fields;
   int curr_struct_offset = 0;
   for (SlotDescriptor* slot: sorted_slots) {
     // IMPALA-3207: Codegen for CHAR is not yet implemented: bail out of codegen here.
@@ -697,7 +696,7 @@ StructType* TupleDescriptor::GetLlvmStruct(LlvmCodeGen* codegen) const {
 
   DCHECK_LE(curr_struct_offset, byte_size_);
   if (curr_struct_offset < byte_size_) {
-    struct_fields.push_back(ArrayType::get(codegen->GetType(TYPE_TINYINT),
+    struct_fields.push_back(llvm::ArrayType::get(codegen->GetType(TYPE_TINYINT),
         byte_size_ - curr_struct_offset));
   }
 
@@ -706,16 +705,15 @@ StructType* TupleDescriptor::GetLlvmStruct(LlvmCodeGen* codegen) const {
   // fields are already aligned because we order the slots by descending size and only
   // have powers-of-two slot sizes. Note that STRING and TIMESTAMP slots both occupy
   // 16 bytes although their useful payload is only 12 bytes.
-  StructType* tuple_struct = StructType::get(codegen->context(),
-      ArrayRef<Type*>(struct_fields), true);
-  const DataLayout& data_layout = codegen->execution_engine()->getDataLayout();
-  const StructLayout* layout = data_layout.getStructLayout(tuple_struct);
+  llvm::StructType* tuple_struct = llvm::StructType::get(codegen->context(),
+      llvm::ArrayRef<llvm::Type*>(struct_fields), true);
+  const llvm::DataLayout& data_layout = codegen->execution_engine()->getDataLayout();
+  const llvm::StructLayout* layout = data_layout.getStructLayout(tuple_struct);
   for (SlotDescriptor* slot: slots()) {
     // Verify that the byte offset in the llvm struct matches the tuple offset
     // computed in the FE.
     DCHECK_EQ(layout->getElementOffset(slot->llvm_field_idx()), slot->tuple_offset());
   }
-  llvm_struct_ = tuple_struct;
   return tuple_struct;
 }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/4c4235f4/be/src/runtime/descriptors.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/descriptors.h b/be/src/runtime/descriptors.h
index 3fd6942..87cce64 100644
--- a/be/src/runtime/descriptors.h
+++ b/be/src/runtime/descriptors.h
@@ -419,7 +419,6 @@ class TupleDescriptor {
   ///   int32_t  min_a;
   ///   int64_t  count_val;
   /// };
-  /// The resulting struct definition is cached.
   llvm::StructType* GetLlvmStruct(LlvmCodeGen* codegen) const;
 
   static const char* LLVM_CLASS_NAME;
@@ -451,9 +450,6 @@ class TupleDescriptor {
   /// collection, empty otherwise.
   SchemaPath tuple_path_;
 
-  /// Cached codegen'd struct type for this tuple desc
-  mutable llvm::StructType* llvm_struct_;
-
   TupleDescriptor(const TTupleDescriptor& tdesc);
   void AddSlot(SlotDescriptor* slot);
 };