Merge remote-tracking branch 'origin/topic/robin/gh-425-record-perf'

- Removed a now-unused-local-variable
- Added std::move() in AssignExpr::SetOp2()

* origin/topic/robin/gh-425-record-perf:
  Avoid unnecessary temporary value when coercing a record that's already the right type.
  Optimize record constructor expression.
  Unify type comparisions for records.

CHANGES

@@ -1,4 +1,26 @@
 
+3.3.0-dev.365 | 2020-10-06 12:19:49 -0700
+
+  * GH-425: Avoid temporary value while coercing records already of the right type. (Robin Sommer, Corelight)
+
+    The combination of this commit with the previous one now lets the examples
+    in GH-425 all execute with the same performance.
+
+  * GH-425: Optimize record constructor expression. (Robin Sommer, Corelight)
+
+    We remove the inheritance from UnaryExpr because we know the type of the
+    operand precisely and can skip a temporary when evaluating the expression.
+
+  * Unify type comparisions for records. (Robin Sommer, Corelight)
+
+    For records, same_type(r1, r2) would not check if the fields'
+    attributes match as well. That seems like an oversight, and some
+    callers of same_type() did indeed add that check on their end. This
+    commit moves the check into same_type() itself. That generally doesn't
+    seem make any differences except for a couple of places validating
+    code, which we update a bit. That in turn leans to slightly different
+    (better?) error messages for a couple of test cases.
+
 3.3.0-dev.361 | 2020-10-06 10:13:37 -0700
 
   * logging/ascii: Support leftover log rotation in non-supervisor setups (Arne Welzel, Corelight)

VERSION

@@ -1 +1 @@
-3.3.0-dev.361
+3.3.0-dev.365

src/Expr.cc

@@ -2164,26 +2164,16 @@ bool AssignExpr::TypeCheck(const AttributesPtr& attrs)
 		if ( attrs )
 			attr_copy = std::make_unique<std::vector<AttrPtr>>(attrs->GetAttrs());
 
-		bool empty_list_assignment = (op2->AsListExpr()->Exprs().empty());
-
 		if ( op1->GetType()->IsSet() )
 			op2 = make_intrusive<SetConstructorExpr>(
-			        cast_intrusive<ListExpr>(op2), std::move(attr_copy));
+			        cast_intrusive<ListExpr>(op2), std::move(attr_copy), op1->GetType());
 		else
 			op2 = make_intrusive<TableConstructorExpr>(
-			        cast_intrusive<ListExpr>(op2), std::move(attr_copy));
+			        cast_intrusive<ListExpr>(op2), std::move(attr_copy), op1->GetType());
 
-		if ( ! empty_list_assignment && ! same_type(op1->GetType(), op2->GetType()) )
+		// The constructor expressions are performing the type
-			{
+		// checking and will set op2 to an error state on failure.
-			if ( op1->GetType()->IsSet() )
+		return ! op2->IsError();
-				ExprError("set type mismatch in assignment");
-			else
-				ExprError("table type mismatch in assignment");
-
-			return false;
-			}
-
-		return true;
 		}
 
 	if ( bt1 == TYPE_VECTOR )
@@ -2207,21 +2197,7 @@ bool AssignExpr::TypeCheck(const AttributesPtr& attrs)
 	     op2->GetType()->Tag() == TYPE_RECORD )
 		{
 		if ( same_type(op1->GetType(), op2->GetType()) )
-			{
+			return true;
-			RecordType* rt1 = op1->GetType()->AsRecordType();
-			RecordType* rt2 = op2->GetType()->AsRecordType();
-
-			// Make sure the attributes match as well.
-			for ( int i = 0; i < rt1->NumFields(); ++i )
-				{
-				const TypeDecl* td1 = rt1->FieldDecl(i);
-				const TypeDecl* td2 = rt2->FieldDecl(i);
-
-				if ( same_attrs(td1->attrs.get(), td2->attrs.get()) )
-					// Everything matches.
-					return true;
-				}
-			}
 
 		// Need to coerce.
 		op2 = make_intrusive<RecordCoerceExpr>(std::move(op2), op1->GetType<RecordType>());
@@ -3077,7 +3053,7 @@ void HasFieldExpr::ExprDescribe(ODesc* d) const
 	}
 
 RecordConstructorExpr::RecordConstructorExpr(ListExprPtr constructor_list)
-	: UnaryExpr(EXPR_RECORD_CONSTRUCTOR, std::move(constructor_list))
+	: Expr(EXPR_RECORD_CONSTRUCTOR), op(std::move(constructor_list))
 	{
 	if ( IsError() )
 		return;
@@ -3130,22 +3106,30 @@ ValPtr RecordConstructorExpr::InitVal(const zeek::Type* t, ValPtr aggr) const
 	return nullptr;
 	}
 
-ValPtr RecordConstructorExpr::Fold(Val* v) const
+ValPtr RecordConstructorExpr::Eval(Frame* f) const
 	{
-	ListVal* lv = v->AsListVal();
+	if ( IsError() )
+		return nullptr;
+
+	const auto& exprs = op->Exprs();
 	auto rt = cast_intrusive<RecordType>(type);
 
-	if ( lv->Length() != rt->NumFields() )
+	if ( exprs.length() != rt->NumFields() )
 		RuntimeErrorWithCallStack("inconsistency evaluating record constructor");
 
 	auto rv = make_intrusive<RecordVal>(std::move(rt));
 
-	for ( int i = 0; i < lv->Length(); ++i )
+	for ( int i = 0; i < exprs.length(); ++i )
-		rv->Assign(i, lv->Idx(i));
+		rv->Assign(i, exprs[i]->Eval(f));
 
 	return rv;
 	}
 
+bool RecordConstructorExpr::IsPure() const
+	{
+	return op->IsPure();
+	}
+
 void RecordConstructorExpr::ExprDescribe(ODesc* d) const
 	{
 	d->Add("[");
@@ -3153,6 +3137,18 @@ void RecordConstructorExpr::ExprDescribe(ODesc* d) const
 	d->Add("]");
 	}
 
+TraversalCode RecordConstructorExpr::Traverse(TraversalCallback* cb) const
+	{
+	TraversalCode tc = cb->PreExpr(this);
+	HANDLE_TC_EXPR_PRE(tc);
+
+	tc = op->Traverse(cb);
+	HANDLE_TC_EXPR_PRE(tc);
+
+	tc = cb->PostExpr(this);
+	HANDLE_TC_EXPR_POST(tc);
+	}
+
 TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list,
                                            std::unique_ptr<std::vector<AttrPtr>> arg_attrs,
                                            TypePtr arg_type)
@@ -3195,17 +3191,18 @@ TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list,
 	const auto& indices = type->AsTableType()->GetIndices()->GetTypes();
 	const ExprPList& cle = op->AsListExpr()->Exprs();
 
-	// check and promote all index expressions in ctor list
+	// check and promote all assign expressions in ctor list
 	for ( const auto& expr : cle )
 		{
 		if ( expr->Tag() != EXPR_ASSIGN )
 			continue;
 
-		Expr* idx_expr = expr->AsAssignExpr()->Op1();
+		auto idx_expr = expr->AsAssignExpr()->Op1();
-
+		auto val_expr = expr->AsAssignExpr()->Op2();
-		if ( idx_expr->Tag() != EXPR_LIST )
+		auto yield_type = GetType()->AsTableType()->Yield().get();
-			continue;
 
+		// Promote LHS
+		assert(idx_expr->Tag() == EXPR_LIST);
 		ExprPList& idx_exprs = idx_expr->AsListExpr()->Exprs();
 
 		if ( idx_exprs.length() != static_cast<int>(indices.size()) )
@@ -3229,6 +3226,19 @@ TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list,
 				}
 
 			ExprError("inconsistent types in table constructor");
+			return;
+			}
+
+		// Promote RHS
+		if ( auto promoted_val = check_and_promote_expr(val_expr, yield_type) )
+			{
+			if ( promoted_val != val_expr )
+				expr->AsAssignExpr()->SetOp2(promoted_val);
+			}
+		else
+			{
+			ExprError("inconsistent types in table constructor");
+			return;
 			}
 		}
 	}
@@ -3328,8 +3338,8 @@ SetConstructorExpr::SetConstructorExpr(ListExprPtr constructor_list,
 			Expr* ce = cle[i];
 			ListExpr* le = ce->AsListExpr();
 
-			if ( ce->Tag() == EXPR_LIST &&
+			assert(ce->Tag() == EXPR_LIST);
-			     check_and_promote_exprs(le, type->AsTableType()->GetIndices().get()) )
+			if ( check_and_promote_exprs(le, type->AsTableType()->GetIndices().get()) )
 				{
 				if ( le != cle[i] )
 					cle.replace(i, le);
@@ -3746,6 +3756,9 @@ ValPtr RecordCoerceExpr::InitVal(const zeek::Type* t, ValPtr aggr) const
 
 ValPtr RecordCoerceExpr::Fold(Val* v) const
 	{
+	if ( same_type(GetType(), Op()->GetType()) )
+		return IntrusivePtr{NewRef{}, v};
+
 	auto val = make_intrusive<RecordVal>(GetType<RecordType>());
 	RecordType* val_type = val->GetType()->AsRecordType();
 
@@ -4985,18 +4998,7 @@ ExprPtr check_and_promote_expr(Expr* const e, zeek::Type* t)
 		RecordType* et_r = et->AsRecordType();
 
 		if ( same_type(t, et) )
-			{
+			return {NewRef{}, e};
-			// Make sure the attributes match as well.
-			for ( int i = 0; i < t_r->NumFields(); ++i )
-				{
-				const TypeDecl* td1 = t_r->FieldDecl(i);
-				const TypeDecl* td2 = et_r->FieldDecl(i);
-
-				if ( same_attrs(td1->attrs.get(), td2->attrs.get()) )
-					// Everything matches perfectly.
-					return {NewRef{}, e};
-				}
-			}
 
 		if ( record_promotion_compatible(t_r, et_r) )
 			return make_intrusive<RecordCoerceExpr>(

src/Expr.h

@@ -504,6 +504,11 @@ public:
 	ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override;
 	bool IsPure() const override;
 
+	void SetOp2(ExprPtr e)
+		{
+		op2 = std::move(e);
+		}
+
 protected:
 	bool TypeCheck(const AttributesPtr& attrs = nullptr);
 	bool TypeCheckArithmetics(TypeTag bt1, TypeTag bt2);
@@ -625,16 +630,25 @@ protected:
 	int field;
 };
 
-class RecordConstructorExpr final : public UnaryExpr {
+class RecordConstructorExpr final : public Expr {
 public:
 	explicit RecordConstructorExpr(ListExprPtr constructor_list);
 	~RecordConstructorExpr() override;
 
+	ListExpr* Op() const	{ return op.get(); }
+
+	ValPtr Eval(Frame* f) const override;
+
+	bool IsPure() const override;
+
+	TraversalCode Traverse(TraversalCallback* cb) const override;
+
 protected:
 	ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override;
-	ValPtr Fold(Val* v) const override;
 
 	void ExprDescribe(ODesc* d) const override;
+
+	ListExprPtr op;
 };
 
 class TableConstructorExpr final : public UnaryExpr {

src/Type.cc

@@ -1690,6 +1690,9 @@ bool same_type(const Type& arg_t1, const Type& arg_t2,
 			if ( (match_record_field_names && ! util::streq(td1->id, td2->id)) ||
 			     ! same_type(td1->type, td2->type, is_init, match_record_field_names) )
 				return false;
+
+			if ( ! same_attrs(td1->attrs.get(), td2->attrs.get()) )
+				return false;
 			}
 
 		return true;

testing/btest/Baseline/language.record-bad-ctor/out

@@ -1,2 +1,2 @@
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 6: no type given (asdfasdf)
+error in /Users/robin/bro/topic/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 6: no type given (asdfasdf)
-expression error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 7: uninitialized list value ($ports=asdfasdf)
+error in /Users/robin/bro/topic/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 7: non-optional field "ports" missing in initialization ([ports=<uninitialized>])

testing/btest/Baseline/language.table-list-assign-type-check/output

@@ -1,3 +1,7 @@
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 15-20: table type mismatch in assignment (service_table_bad_yield = table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi))
+error in count and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 16: arithmetic mixed with non-arithmetic (count and Internal Web Server)
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 23-28: table type mismatch in assignment (service_table_bad_index = table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi))
+error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 15-20: inconsistent types in table constructor (table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi))
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: set type mismatch in assignment (test_set_bad = set(1, 2, 3))
+error in count and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 24: arithmetic mixed with non-arithmetic (count and 80)
+error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 23-28: inconsistent types in table constructor (table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi))
+error in string and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: arithmetic mixed with non-arithmetic (string and 1)
+error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31 and string: type mismatch (1 and string)
+error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: inconsistent type in set constructor (set(1, 2, 3))