From f866252e5ee3524af1502899b63dd3d27e7b9264 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Mon, 10 Apr 2023 11:42:48 -0700 Subject: [PATCH 1/8] remove redundant record coercions --- src/Expr.cc | 4 ++++ testing/btest/Baseline/plugins.hooks/output | 6 +++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 68933eb228..1a7c06020a 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4215,6 +4215,10 @@ TableCoerceExpr::TableCoerceExpr(ExprPtr arg_op, TableTypePtr tt, bool type_chec SetError(); return; } + + if ( op->Tag() == EXPR_TABLE_COERCE && op->GetType() == tt ) + // Avoid double-coercion. + op = op->GetOp1(); } SetType(std::move(tt)); diff --git a/testing/btest/Baseline/plugins.hooks/output b/testing/btest/Baseline/plugins.hooks/output index 998f804f04..33b1951ae4 100644 --- a/testing/btest/Baseline/plugins.hooks/output +++ b/testing/btest/Baseline/plugins.hooks/output @@ -735,7 +735,7 @@ 0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugin, , (SumStats::STD_DEV, lambda_<5704045257244168718>{ SumStats::calc_std_dev(SumStats::rv)})) -> 0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugin, , (SumStats::SUM, lambda_<7459411543525688824>{ SumStats::rv$sum = SumStats::rv$sum + SumStats::val})) -> 0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugin, , (SumStats::TOPK, lambda_<6366101205573988923>{ topk_add(SumStats::rv$topk, to_any_coerceSumStats::obs)})) -> -0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugin, , (SumStats::UNIQUE, lambda_<6609886180724383051>{ if (!SumStats::rv?$unique_vals) SumStats::rv$unique_vals = (coerce (coerce set() to set[SumStats::Observation]) to set[SumStats::Observation])if (SumStats::r?$unique_max) SumStats::rv$unique_max = SumStats::r$unique_maxif (!SumStats::r?$unique_max || sizeofSumStats::rv$unique_vals <= SumStats::r$unique_max) add SumStats::rv$unique_vals[SumStats::obs]SumStats::rv$unique = sizeofSumStats::rv$unique_vals})) -> +0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugin, , (SumStats::UNIQUE, lambda_<11310474105220719698>{ if (!SumStats::rv?$unique_vals) SumStats::rv$unique_vals = (coerce set() to set[SumStats::Observation])if (SumStats::r?$unique_max) SumStats::rv$unique_max = SumStats::r$unique_maxif (!SumStats::r?$unique_max || sizeofSumStats::rv$unique_vals <= SumStats::r$unique_max) add SumStats::rv$unique_vals[SumStats::obs]SumStats::rv$unique = sizeofSumStats::rv$unique_vals})) -> 0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugin, , (SumStats::VARIANCE, lambda_<5978956599776442208>{ if (1 < SumStats::rv$num) SumStats::rv$var_s = SumStats::rv$var_s + ((SumStats::val - SumStats::rv$prev_avg) * (SumStats::val - SumStats::rv$average))SumStats::calc_variance(SumStats::rv)SumStats::rv$prev_avg = SumStats::rv$average})) -> 0.000000 MetaHookPost CallFunction(SumStats::register_observe_plugins, , ()) -> 0.000000 MetaHookPost CallFunction(Supervisor::__is_supervisor, , ()) -> @@ -2313,7 +2313,7 @@ 0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugin, , (SumStats::STD_DEV, lambda_<5704045257244168718>{ SumStats::calc_std_dev(SumStats::rv)})) 0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugin, , (SumStats::SUM, lambda_<7459411543525688824>{ SumStats::rv$sum = SumStats::rv$sum + SumStats::val})) 0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugin, , (SumStats::TOPK, lambda_<6366101205573988923>{ topk_add(SumStats::rv$topk, to_any_coerceSumStats::obs)})) -0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugin, , (SumStats::UNIQUE, lambda_<6609886180724383051>{ if (!SumStats::rv?$unique_vals) SumStats::rv$unique_vals = (coerce (coerce set() to set[SumStats::Observation]) to set[SumStats::Observation])if (SumStats::r?$unique_max) SumStats::rv$unique_max = SumStats::r$unique_maxif (!SumStats::r?$unique_max || sizeofSumStats::rv$unique_vals <= SumStats::r$unique_max) add SumStats::rv$unique_vals[SumStats::obs]SumStats::rv$unique = sizeofSumStats::rv$unique_vals})) +0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugin, , (SumStats::UNIQUE, lambda_<11310474105220719698>{ if (!SumStats::rv?$unique_vals) SumStats::rv$unique_vals = (coerce set() to set[SumStats::Observation])if (SumStats::r?$unique_max) SumStats::rv$unique_max = SumStats::r$unique_maxif (!SumStats::r?$unique_max || sizeofSumStats::rv$unique_vals <= SumStats::r$unique_max) add SumStats::rv$unique_vals[SumStats::obs]SumStats::rv$unique = sizeofSumStats::rv$unique_vals})) 0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugin, , (SumStats::VARIANCE, lambda_<5978956599776442208>{ if (1 < SumStats::rv$num) SumStats::rv$var_s = SumStats::rv$var_s + ((SumStats::val - SumStats::rv$prev_avg) * (SumStats::val - SumStats::rv$average))SumStats::calc_variance(SumStats::rv)SumStats::rv$prev_avg = SumStats::rv$average})) 0.000000 MetaHookPre CallFunction(SumStats::register_observe_plugins, , ()) 0.000000 MetaHookPre CallFunction(Supervisor::__is_supervisor, , ()) @@ -3890,7 +3890,7 @@ 0.000000 | HookCallFunction SumStats::register_observe_plugin(SumStats::STD_DEV, lambda_<5704045257244168718>{ SumStats::calc_std_dev(SumStats::rv)}) 0.000000 | HookCallFunction SumStats::register_observe_plugin(SumStats::SUM, lambda_<7459411543525688824>{ SumStats::rv$sum = SumStats::rv$sum + SumStats::val}) 0.000000 | HookCallFunction SumStats::register_observe_plugin(SumStats::TOPK, lambda_<6366101205573988923>{ topk_add(SumStats::rv$topk, to_any_coerceSumStats::obs)}) -0.000000 | HookCallFunction SumStats::register_observe_plugin(SumStats::UNIQUE, lambda_<6609886180724383051>{ if (!SumStats::rv?$unique_vals) SumStats::rv$unique_vals = (coerce (coerce set() to set[SumStats::Observation]) to set[SumStats::Observation])if (SumStats::r?$unique_max) SumStats::rv$unique_max = SumStats::r$unique_maxif (!SumStats::r?$unique_max || sizeofSumStats::rv$unique_vals <= SumStats::r$unique_max) add SumStats::rv$unique_vals[SumStats::obs]SumStats::rv$unique = sizeofSumStats::rv$unique_vals}) +0.000000 | HookCallFunction SumStats::register_observe_plugin(SumStats::UNIQUE, lambda_<11310474105220719698>{ if (!SumStats::rv?$unique_vals) SumStats::rv$unique_vals = (coerce set() to set[SumStats::Observation])if (SumStats::r?$unique_max) SumStats::rv$unique_max = SumStats::r$unique_maxif (!SumStats::r?$unique_max || sizeofSumStats::rv$unique_vals <= SumStats::r$unique_max) add SumStats::rv$unique_vals[SumStats::obs]SumStats::rv$unique = sizeofSumStats::rv$unique_vals}) 0.000000 | HookCallFunction SumStats::register_observe_plugin(SumStats::VARIANCE, lambda_<5978956599776442208>{ if (1 < SumStats::rv$num) SumStats::rv$var_s = SumStats::rv$var_s + ((SumStats::val - SumStats::rv$prev_avg) * (SumStats::val - SumStats::rv$average))SumStats::calc_variance(SumStats::rv)SumStats::rv$prev_avg = SumStats::rv$average}) 0.000000 | HookCallFunction SumStats::register_observe_plugins() 0.000000 | HookCallFunction Supervisor::__is_supervisor() From 4600ca41f656ecc1d5ef7aa20f34e35453b0be46 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Mon, 10 Apr 2023 11:43:19 -0700 Subject: [PATCH 2/8] logging speedup by switching to raw record access --- src/Val.h | 8 ++++ src/logging/Manager.cc | 83 ++++++++++++++++++++++++++++-------------- src/logging/Manager.h | 2 +- 3 files changed, 65 insertions(+), 28 deletions(-) diff --git a/src/Val.h b/src/Val.h index e73536f127..01e4a201ee 100644 --- a/src/Val.h +++ b/src/Val.h @@ -52,9 +52,15 @@ class HashKey; class ValTrace; class ZBody; +class CPPRuntime; } // namespace detail +namespace logging + { +class Manager; + } + namespace run_state { @@ -1403,8 +1409,10 @@ public: static void DoneParsing(); protected: + friend class zeek::logging::Manager; friend class zeek::detail::ValTrace; friend class zeek::detail::ZBody; + friend class zeek::detail::CPPRuntime; RecordValPtr DoCoerceTo(RecordTypePtr other, bool allow_orphaning) const; diff --git a/src/logging/Manager.cc b/src/logging/Manager.cc index 2b39aa4ff2..f4d55086df 100644 --- a/src/logging/Manager.cc +++ b/src/logging/Manager.cc @@ -973,11 +973,8 @@ bool Manager::Write(EnumVal* id, RecordVal* columns_arg) return true; } -threading::Value* Manager::ValToLogVal(Val* val, Type* ty) +threading::Value* Manager::ValToLogVal(std::optional& val, Type* ty) { - if ( ! ty ) - ty = val->GetType().get(); - if ( ! val ) return new threading::Value(ty->Tag(), false); @@ -987,12 +984,12 @@ threading::Value* Manager::ValToLogVal(Val* val, Type* ty) { case TYPE_BOOL: case TYPE_INT: - lval->val.int_val = val->InternalInt(); + lval->val.int_val = val->AsInt(); break; case TYPE_ENUM: { - const char* s = val->GetType()->AsEnumType()->Lookup(val->InternalInt()); + const char* s = ty->AsEnumType()->Lookup(val->AsInt()); if ( s ) { @@ -1002,7 +999,8 @@ threading::Value* Manager::ValToLogVal(Val* val, Type* ty) else { - val->GetType()->Error("enum type does not contain value", val); + auto err_msg = "enum type does not contain value:" + std::to_string(val->AsInt()); + ty->Error(err_msg.c_str()); lval->val.string_val.data = util::copy_string(""); lval->val.string_val.length = 0; } @@ -1010,31 +1008,44 @@ threading::Value* Manager::ValToLogVal(Val* val, Type* ty) } case TYPE_COUNT: - lval->val.uint_val = val->InternalUnsigned(); + lval->val.uint_val = val->AsCount(); break; case TYPE_PORT: - lval->val.port_val.port = val->AsPortVal()->Port(); - lval->val.port_val.proto = val->AsPortVal()->PortType(); + { + auto p = val->AsCount(); + + auto pt = TRANSPORT_UNKNOWN; + auto pm = p & PORT_SPACE_MASK; + if ( pm == TCP_PORT_MASK ) + pt = TRANSPORT_TCP; + else if ( pm == UDP_PORT_MASK ) + pt = TRANSPORT_UDP; + else if ( pm == ICMP_PORT_MASK ) + pt = TRANSPORT_ICMP; + + lval->val.port_val.port = p & ~PORT_SPACE_MASK; + lval->val.port_val.proto = pt; break; + } case TYPE_SUBNET: - val->AsSubNet().ConvertToThreadingValue(&lval->val.subnet_val); + val->AsSubNet()->Get().ConvertToThreadingValue(&lval->val.subnet_val); break; case TYPE_ADDR: - val->AsAddr().ConvertToThreadingValue(&lval->val.addr_val); + val->AsAddr()->Get().ConvertToThreadingValue(&lval->val.addr_val); break; case TYPE_DOUBLE: case TYPE_TIME: case TYPE_INTERVAL: - lval->val.double_val = val->InternalDouble(); + lval->val.double_val = val->AsDouble(); break; case TYPE_STRING: { - const String* s = val->AsString(); + const String* s = val->AsString()->AsString(); char* buf = new char[s->Len()]; memcpy(buf, s->Bytes(), s->Len()); @@ -1065,31 +1076,44 @@ threading::Value* Manager::ValToLogVal(Val* val, Type* ty) case TYPE_TABLE: { - auto set = val->AsTableVal()->ToPureListVal(); + auto tbl = val->AsTable(); + auto set = tbl->ToPureListVal(); + if ( ! set ) // ToPureListVal has reported an internal warning // already. Just keep going by making something up. set = make_intrusive(TYPE_INT); + auto tbl_t = cast_intrusive(tbl->GetType()); + auto& set_t = tbl_t->GetIndexTypes()[0]; + bool is_managed = ZVal::IsManagedType(set_t); + lval->val.set_val.size = set->Length(); lval->val.set_val.vals = new threading::Value*[lval->val.set_val.size]; for ( zeek_int_t i = 0; i < lval->val.set_val.size; i++ ) - lval->val.set_val.vals[i] = ValToLogVal(set->Idx(i).get()); + { + std::optional s_i = ZVal(set->Idx(i), set_t); + lval->val.set_val.vals[i] = ValToLogVal(s_i, set_t.get()); + if ( is_managed ) + ZVal::DeleteManagedType(*s_i); + } break; } case TYPE_VECTOR: { - VectorVal* vec = val->AsVectorVal(); + VectorVal* vec = val->AsVector(); lval->val.vector_val.size = vec->Size(); lval->val.vector_val.vals = new threading::Value*[lval->val.vector_val.size]; + auto& vv = vec->RawVec(); + auto& vt = vec->GetType()->Yield(); + for ( zeek_int_t i = 0; i < lval->val.vector_val.size; i++ ) { - lval->val.vector_val.vals[i] = ValToLogVal(vec->ValAt(i).get(), - vec->GetType()->Yield().get()); + lval->val.vector_val.vals[i] = ValToLogVal((*vv)[i], vt.get()); } break; @@ -1118,7 +1142,8 @@ threading::Value** Manager::RecordToFilterVals(Stream* stream, Filter* filter, R for ( int i = 0; i < filter->num_fields; ++i ) { - Val* val; + std::optional val; + Type* vt; if ( i < filter->num_ext_fields ) { if ( ! ext_rec ) @@ -1128,21 +1153,23 @@ threading::Value** Manager::RecordToFilterVals(Stream* stream, Filter* filter, R continue; } - val = ext_rec.get(); + val = ZVal(ext_rec.get()); + vt = ext_rec->GetType().get(); } else - val = columns; + { + val = ZVal(columns); + vt = columns->GetType().get(); + } // For each field, first find the right value, which can // potentially be nested inside other records. list& indices = filter->indices[i]; - ValPtr val_ptr; - for ( list::iterator j = indices.begin(); j != indices.end(); ++j ) { - val_ptr = val->AsRecordVal()->GetField(*j); - val = val_ptr.get(); + auto vr = val->AsRecord(); + val = vr->RawOptField(*j); if ( ! val ) { @@ -1150,10 +1177,12 @@ threading::Value** Manager::RecordToFilterVals(Stream* stream, Filter* filter, R vals[i] = new threading::Value(filter->fields[i]->type, false); break; } + + vt = cast_intrusive(vr->GetType())->GetFieldType(*j).get(); } if ( val ) - vals[i] = ValToLogVal(val); + vals[i] = ValToLogVal(val, vt); } return vals; diff --git a/src/logging/Manager.h b/src/logging/Manager.h index 6f926242f3..cb2e554d5c 100644 --- a/src/logging/Manager.h +++ b/src/logging/Manager.h @@ -291,7 +291,7 @@ private: threading::Value** RecordToFilterVals(Stream* stream, Filter* filter, RecordVal* columns); - threading::Value* ValToLogVal(Val* val, Type* ty = nullptr); + threading::Value* ValToLogVal(std::optional& val, Type* ty); Stream* FindStream(EnumVal* id); void RemoveDisabledWriters(Stream* stream); void InstallRotationTimer(WriterInfo* winfo); From 2e2afa5e113b5f3f951427c85c7fb113a280f0e9 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Mon, 10 Apr 2023 11:43:40 -0700 Subject: [PATCH 3/8] compile-scripts-to-C++ speedups by switching to raw record access --- src/script_opt/CPP/Compile.h | 3 ++- src/script_opt/CPP/Exprs.cc | 45 +++++++++++++++++++++++++-------- src/script_opt/CPP/RuntimeOps.h | 44 +++++++++++++++++++++++++++++++- 3 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/script_opt/CPP/Compile.h b/src/script_opt/CPP/Compile.h index 9a99db499c..6b110a00de 100644 --- a/src/script_opt/CPP/Compile.h +++ b/src/script_opt/CPP/Compile.h @@ -820,7 +820,8 @@ private: std::string GenIndexAssign(const ExprPtr& lhs, const ExprPtr& rhs, const std::string& rhs_val_ptr, GenType gt, bool top_level); std::string GenFieldAssign(const ExprPtr& lhs, const ExprPtr& rhs, - const std::string& rhs_val_ptr, GenType gt, bool top_level); + const std::string& rhs_native, const std::string& rhs_val_ptr, + GenType gt, bool top_level); std::string GenListAssign(const ExprPtr& lhs, const ExprPtr& rhs); // Support for element-by-element vector operations. diff --git a/src/script_opt/CPP/Exprs.cc b/src/script_opt/CPP/Exprs.cc index 6dd82356a5..b0ec6cd8e4 100644 --- a/src/script_opt/CPP/Exprs.cc +++ b/src/script_opt/CPP/Exprs.cc @@ -396,13 +396,35 @@ string CPPCompile::GenInExpr(const Expr* e, GenType gt) string CPPCompile::GenFieldExpr(const FieldExpr* fe, GenType gt) { + auto& t = fe->GetType(); auto r = fe->GetOp1(); auto f = fe->Field(); auto f_s = GenField(r, f); - auto gen = string("field_access__CPP(") + GenExpr(r, GEN_VAL_PTR) + ", " + f_s + ")"; + string gen; - return GenericValPtrToGT(gen, fe->GetType(), gt); + if ( IsNativeType(t) ) + { + auto nt = TypeName(t); + gen = string("field_access_") + nt + "__CPP(" + GenExpr(r, GEN_VAL_PTR) + ", " + f_s + ")"; + return NativeToGT(gen, t, gt); + } + + switch ( t->Tag() ) + { + case TYPE_FILE: + case TYPE_FUNC: + case TYPE_VOID: + gen = string("field_access__CPP(") + GenExpr(r, GEN_VAL_PTR) + ", " + f_s + ")"; + return GenericValPtrToGT(gen, t, gt); + + default: + { + auto nt = TypeName(t); + return string("field_access_") + nt + "__CPP(" + GenExpr(r, GEN_VAL_PTR) + ", " + f_s + + ")"; + } + } } string CPPCompile::GenHasFieldExpr(const HasFieldExpr* hfe, GenType gt) @@ -411,8 +433,7 @@ string CPPCompile::GenHasFieldExpr(const HasFieldExpr* hfe, GenType gt) auto f = hfe->Field(); auto f_s = GenField(r, f); - // Need to use accessors for native types. - auto gen = string("(") + GenExpr(r, GEN_DONT_CARE) + "->GetField(" + f_s + ") != nullptr)"; + auto gen = GenExpr(r, GEN_DONT_CARE) + "->HasField(" + f_s + ")"; return NativeToGT(gen, hfe->GetType(), gt); } @@ -1090,7 +1111,7 @@ string CPPCompile::GenAssign(const ExprPtr& lhs, const ExprPtr& rhs, const strin return GenIndexAssign(lhs, rhs, rhs_val_ptr, gt, top_level); case EXPR_FIELD: - return GenFieldAssign(lhs, rhs, rhs_val_ptr, gt, top_level); + return GenFieldAssign(lhs, rhs, rhs_native, rhs_val_ptr, gt, top_level); case EXPR_LIST: return GenListAssign(lhs, rhs); @@ -1159,20 +1180,24 @@ string CPPCompile::GenIndexAssign(const ExprPtr& lhs, const ExprPtr& rhs, const return gen; } -string CPPCompile::GenFieldAssign(const ExprPtr& lhs, const ExprPtr& rhs, const string& rhs_val_ptr, - GenType gt, bool top_level) +string CPPCompile::GenFieldAssign(const ExprPtr& lhs, const ExprPtr& rhs, const string& rhs_native, + const string& rhs_val_ptr, GenType gt, bool top_level) { auto rec = lhs->GetOp1(); auto rec_gen = GenExpr(rec, GEN_VAL_PTR); auto field = GenField(rec, lhs->AsFieldExpr()->Field()); - if ( top_level ) - return rec_gen + "->Assign(" + field + ", " + rhs_val_ptr + ")"; - else + if ( ! top_level ) { auto gen = string("assign_field__CPP(") + rec_gen + ", " + field + ", " + rhs_val_ptr + ")"; return GenericValPtrToGT(gen, rhs->GetType(), gt); } + + auto rt = rhs ? rhs->GetType() : nullptr; + if ( rt && (IsNativeType(rt) || rt->Tag() == TYPE_STRING) ) + return rec_gen + "->Assign(" + field + ", " + rhs_native + ")"; + else + return rec_gen + "->Assign(" + field + ", " + rhs_val_ptr + ")"; } string CPPCompile::GenListAssign(const ExprPtr& lhs, const ExprPtr& rhs) diff --git a/src/script_opt/CPP/RuntimeOps.h b/src/script_opt/CPP/RuntimeOps.h index 85cc61eeca..4318e7c7ff 100644 --- a/src/script_opt/CPP/RuntimeOps.h +++ b/src/script_opt/CPP/RuntimeOps.h @@ -5,7 +5,7 @@ #pragma once #include "zeek/Frame.h" -#include "zeek/Val.h" +#include "zeek/OpaqueVal.h" #include "zeek/script_opt/CPP/Func.h" namespace zeek @@ -16,6 +16,12 @@ using SubNetValPtr = IntrusivePtr; namespace detail { +class CPPRuntime + { +public: + static auto RawOptField(const RecordValPtr& rv, int field) { return rv->RawOptField(field); } + }; + // Returns the concatenation of the given strings. extern StringValPtr str_concat__CPP(const String* s1, const String* s2); @@ -109,6 +115,42 @@ inline ValPtr field_access__CPP(const RecordValPtr& rec, int field) return v; } +#define NATIVE_FIELD_ACCESS(type, zaccessor, vaccessor) \ + inline type field_access_##type##__CPP(const RecordValPtr& r, int field) \ + { \ + auto rv = CPPRuntime::RawOptField(r, field); \ + if ( rv ) \ + return (*rv).zaccessor(); \ + return field_access__CPP(r, field)->vaccessor(); \ + } + +NATIVE_FIELD_ACCESS(bool, AsInt, AsBool) +NATIVE_FIELD_ACCESS(int, AsInt, AsInt) +NATIVE_FIELD_ACCESS(zeek_int_t, AsInt, AsInt) +NATIVE_FIELD_ACCESS(zeek_uint_t, AsCount, AsCount) +NATIVE_FIELD_ACCESS(double, AsDouble, AsDouble) + +#define VP_FIELD_ACCESS(type, zaccessor) \ + inline type##Ptr field_access_##type##__CPP(const RecordValPtr& r, int field) \ + { \ + auto rv = CPPRuntime::RawOptField(r, field); \ + if ( rv ) \ + return {NewRef{}, rv->zaccessor()}; \ + return cast_intrusive(field_access__CPP(r, field)); \ + } + +VP_FIELD_ACCESS(StringVal, AsString) +VP_FIELD_ACCESS(AddrVal, AsAddr) +VP_FIELD_ACCESS(SubNetVal, AsSubNet) +VP_FIELD_ACCESS(ListVal, AsList) +VP_FIELD_ACCESS(OpaqueVal, AsOpaque) +VP_FIELD_ACCESS(PatternVal, AsPattern) +VP_FIELD_ACCESS(TableVal, AsTable) +VP_FIELD_ACCESS(RecordVal, AsRecord) +VP_FIELD_ACCESS(VectorVal, AsVector) +VP_FIELD_ACCESS(TypeVal, AsType) +VP_FIELD_ACCESS(Val, AsAny) + // Each of the following executes the assignment "v1[v2] = v3" for // tables/vectors/strings. extern ValPtr assign_to_index__CPP(TableValPtr v1, ValPtr v2, ValPtr v3); From 0787c130d03ed8319039c704869815a58c3a9769 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Mon, 10 Apr 2023 11:44:11 -0700 Subject: [PATCH 4/8] optimize record construction by deferring initializations of aggregates --- src/Type.cc | 217 ++++++++++++++++++++++++---------------------------- src/Type.h | 35 ++++++--- src/Val.cc | 43 +++++++---- src/Val.h | 52 +++++++++---- src/ZVal.h | 1 + 5 files changed, 190 insertions(+), 158 deletions(-) diff --git a/src/Type.cc b/src/Type.cc index f38a03564b..e4ac1968a5 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -992,39 +992,102 @@ void TypeDecl::DescribeReST(ODesc* d, bool roles_only) const } } -// The following tracks how to initialize a given field, for fast execution -// of Create(). - -class FieldInit +class DirectFieldInit final : public FieldInit { public: - // The type of initialization for the field. - enum + DirectFieldInit(ZVal _init_val) : init_val(_init_val) { } + + ZVal Generate() const override { return init_val; } + +private: + ZVal init_val; + }; + +class DirectManagedFieldInit final : public FieldInit + { +public: + DirectManagedFieldInit(ZVal _init_val) : init_val(_init_val) { } + ~DirectManagedFieldInit() { ZVal::DeleteManagedType(init_val); } + + ZVal Generate() const override { - R_INIT_NONE, // skip this entry + zeek::Ref(init_val.ManagedVal()); + return init_val; + } - R_INIT_DIRECT, // look in direct_init for raw value - R_INIT_DIRECT_MANAGED, // same, but managed type +private: + ZVal init_val; + }; - R_INIT_DEF, // look in def_expr for expression +class ExprFieldInit final : public FieldInit + { +public: + // Initialization requires evaluating the given expression, + // yielding the a value of the given type (which might require + // coercion for some records). + ExprFieldInit(detail::ExprPtr _init_expr, TypePtr _init_type) + : init_expr(std::move(_init_expr)), init_type(std::move(_init_type)) + { + if ( init_type->Tag() == TYPE_RECORD && ! same_type(init_expr->GetType(), init_type) ) + coerce_type = cast_intrusive(init_type); + } - R_INIT_RECORD, // field requires a new record - R_INIT_TABLE, // field requires a new table/set - R_INIT_VECTOR, // field requires a new vector - } init_type = R_INIT_NONE; + ZVal Generate() const override + { + auto v = init_expr->Eval(nullptr); + if ( ! v ) + { + reporter->Error("failed &default in record creation"); + return ZVal(); + } - bool def_coerce = false; // whether coercion's required + if ( coerce_type ) + v = v->AsRecordVal()->CoerceTo(coerce_type); - // For R_INIT_DIRECT/R_INIT_DIRECT_MANAGED: - ZVal direct_init; + return ZVal(v, init_type); + } - detail::ExprPtr def_expr; - TypePtr def_type; +private: + detail::ExprPtr init_expr; + TypePtr init_type; + RecordTypePtr coerce_type; // non-nil iff coercion is required + }; - RecordTypePtr r_type; // for R_INIT_RECORD - TableTypePtr t_type; // for R_INIT_TABLE - detail::AttributesPtr attrs; // attributes for R_INIT_TABLE - VectorTypePtr v_type; // for R_INIT_VECTOR +class RecordFieldInit final : public FieldInit + { +public: + RecordFieldInit(RecordTypePtr _init_type) : init_type(std::move(_init_type)) { } + + ZVal Generate() const override { return ZVal(new RecordVal(init_type)); } + +private: + RecordTypePtr init_type; + }; + +class TableFieldInit final : public FieldInit + { +public: + TableFieldInit(TableTypePtr _init_type, detail::AttributesPtr _attrs) + : init_type(std::move(_init_type)), attrs(std::move(_attrs)) + { + } + + ZVal Generate() const override { return ZVal(new TableVal(init_type, attrs)); } + +private: + TableTypePtr init_type; + detail::AttributesPtr attrs; + }; + +class VectorFieldInit final : public FieldInit + { +public: + VectorFieldInit(VectorTypePtr _init_type) : init_type(std::move(_init_type)) { } + + ZVal Generate() const override { return ZVal(new VectorVal(init_type)); } + +private: + VectorTypePtr init_type; }; RecordType::RecordType(type_decl_list* arg_types) : Type(TYPE_RECORD) @@ -1064,7 +1127,8 @@ RecordType::~RecordType() } for ( auto fi : field_inits ) - delete fi; + if ( fi ) + delete *fi; } void RecordType::AddField(unsigned int field, const TypeDecl* td) @@ -1074,53 +1138,43 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) managed_fields.push_back(ZVal::IsManagedType(td->type)); - auto init = new FieldInit(); - init->init_type = FieldInit::R_INIT_NONE; - - init->attrs = td->attrs; - // We defer error-checking until here so that we can keep field_inits // and managed_fields correctly tracking the associated fields. if ( field_ids.count(td->id) != 0 ) { reporter->Error("duplicate field '%s' found in record definition", td->id); - field_inits.push_back(init); + field_inits.push_back(std::nullopt); return; } field_ids.insert(std::string(td->id)); - auto a = init->attrs; - + auto a = td->attrs; auto type = td->type; auto def_attr = a ? a->Find(detail::ATTR_DEFAULT) : nullptr; auto def_expr = def_attr ? def_attr->GetExpr() : nullptr; + std::optional init; + if ( def_expr && ! IsErrorType(type->Tag()) ) { - if ( type->Tag() == TYPE_RECORD && def_expr->GetType()->Tag() == TYPE_RECORD && - ! same_type(def_expr->GetType(), type) ) - init->def_coerce = true; - if ( def_expr->Tag() == detail::EXPR_CONST ) { auto v = def_expr->Eval(nullptr); + auto zv = ZVal(v, type); if ( ZVal::IsManagedType(type) ) - init->init_type = FieldInit::R_INIT_DIRECT_MANAGED; + init = new DirectManagedFieldInit(zv); else - init->init_type = FieldInit::R_INIT_DIRECT; - - init->direct_init = ZVal(v, type); + init = new DirectFieldInit(zv); } else { - init->init_type = FieldInit::R_INIT_DEF; - init->def_expr = def_expr; - init->def_type = def_expr->GetType(); + auto efi = new ExprFieldInit(def_expr, type); + field_expr_inits.emplace_back(std::make_pair(field, efi)); } } @@ -1129,22 +1183,13 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) TypeTag tag = type->Tag(); if ( tag == TYPE_RECORD ) - { - init->init_type = FieldInit::R_INIT_RECORD; - init->r_type = cast_intrusive(type); - } + init = new RecordFieldInit(cast_intrusive(type)); else if ( tag == TYPE_TABLE ) - { - init->init_type = FieldInit::R_INIT_TABLE; - init->t_type = cast_intrusive(type); - } + init = new TableFieldInit(cast_intrusive(type), a); else if ( tag == TYPE_VECTOR ) - { - init->init_type = FieldInit::R_INIT_VECTOR; - init->v_type = cast_intrusive(type); - } + init = new VectorFieldInit(cast_intrusive(type)); } field_inits.push_back(init); @@ -1342,68 +1387,6 @@ void RecordType::AddFieldsDirectly(const type_decl_list& others, bool add_log_at num_fields = types->length(); } -void RecordType::Create(std::vector>& r) const - { - int n = NumFields(); - - for ( int i = 0; i < n; ++i ) - { - auto* init = field_inits[i]; - - ZVal r_i; - - switch ( init->init_type ) - { - case FieldInit::R_INIT_NONE: - r.push_back(std::nullopt); - continue; - - case FieldInit::R_INIT_DIRECT: - r_i = init->direct_init; - break; - - case FieldInit::R_INIT_DIRECT_MANAGED: - r_i = init->direct_init; - zeek::Ref(r_i.ManagedVal()); - break; - - case FieldInit::R_INIT_DEF: - { - auto v = init->def_expr->Eval(nullptr); - if ( v ) - { - const auto& t = init->def_type; - - if ( init->def_coerce ) - { - auto rt = cast_intrusive(t); - v = v->AsRecordVal()->CoerceTo(rt); - } - - r_i = ZVal(v, t); - } - else - reporter->Error("failed &default in record creation"); - } - break; - - case FieldInit::R_INIT_RECORD: - r_i = ZVal(new RecordVal(init->r_type)); - break; - - case FieldInit::R_INIT_TABLE: - r_i = ZVal(new TableVal(init->t_type, init->attrs)); - break; - - case FieldInit::R_INIT_VECTOR: - r_i = ZVal(new VectorVal(init->v_type)); - break; - } - - r.push_back(r_i); - } - } - void RecordType::DescribeFields(ODesc* d) const { if ( d->IsReadable() ) diff --git a/src/Type.h b/src/Type.h index b967524be4..f3edba43fb 100644 --- a/src/Type.h +++ b/src/Type.h @@ -15,14 +15,15 @@ #include "zeek/IntrusivePtr.h" #include "zeek/Obj.h" #include "zeek/Traverse.h" +#include "zeek/ZVal.h" #include "zeek/ZeekList.h" namespace zeek { class Val; -union ZVal; class EnumVal; +class RecordVal; class TableVal; using ValPtr = IntrusivePtr; using EnumValPtr = IntrusivePtr; @@ -600,10 +601,16 @@ public: using type_decl_list = PList; -// The following tracks how to initialize a given field. We don't define -// it here because it requires pulling in a bunch of low-level headers that -// would be nice to avoid. -class FieldInit; +// The following tracks how to initialize a given field. + +class FieldInit + { +public: + virtual ~FieldInit() { } + + // Return the initialization value of the field. + virtual ZVal Generate() const = 0; + }; class RecordType final : public Type { @@ -683,12 +690,8 @@ public: void AddFieldsDirectly(const type_decl_list& types, bool add_log_attr = false); - /** - * - * Populates a new instance of the record with its initial values. - * @param r The record's underlying value vector. - */ - void Create(std::vector>& r) const; + const auto& FieldInits() const { return field_inits; } + const auto& FieldExprInits() const { return field_expr_inits; } void DescribeReST(ODesc* d, bool roles_only = false) const override; void DescribeFields(ODesc* d) const; @@ -719,7 +722,15 @@ protected: // Maps each field to how to initialize it. Uses pointers due to // keeping the FieldInit definition private to Type.cc (see above). - std::vector field_inits; + std::vector> field_inits; + + // Holds initializations defined in terms of evaluating expressions, + // in pairs (we use pairs instead of a vector + // with per-field expressions because such expressions are not often + // used). These need to be evaluated at record construction time, + // rather than deferring until first use, because the value of the + // expression can change between the two. + std::vector> field_expr_inits; // If we were willing to bound the size of records, then we could // use std::bitset here instead. diff --git a/src/Val.cc b/src/Val.cc index 28c2dcbad7..7f45e13926 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -2756,25 +2756,32 @@ RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t), is_managed(t-> int n = rt->NumFields(); - record_val = new std::vector>; - record_val->reserve(n); - if ( run_state::is_parsing ) parse_time_records[rt.get()].emplace_back(NewRef{}, this); + record_val = new std::vector>; + if ( init_fields ) { - try + record_val->resize(n); + + for ( auto& e : rt->FieldExprInits() ) { - rt->Create(*record_val); - } - catch ( InterpreterException& e ) - { - if ( run_state::is_parsing ) - parse_time_records[rt.get()].pop_back(); - throw; + try + { + (*record_val)[e.first] = e.second->Generate(); + } + catch ( InterpreterException& e ) + { + if ( run_state::is_parsing ) + parse_time_records[rt.get()].pop_back(); + throw; + } } } + + else + record_val->reserve(n); } RecordVal::~RecordVal() @@ -2782,8 +2789,11 @@ RecordVal::~RecordVal() auto n = record_val->size(); for ( unsigned int i = 0; i < n; ++i ) - if ( HasField(i) && IsManaged(i) ) - ZVal::DeleteManagedType(*(*record_val)[i]); + { + auto f_i = (*record_val)[i]; + if ( f_i && IsManaged(i) ) + ZVal::DeleteManagedType(*f_i); + } delete record_val; } @@ -2809,12 +2819,13 @@ void RecordVal::Assign(int field, ValPtr new_val) void RecordVal::Remove(int field) { - if ( HasField(field) ) + auto& f_i = (*record_val)[field]; + if ( f_i ) { if ( IsManaged(field) ) - ZVal::DeleteManagedType(*(*record_val)[field]); + ZVal::DeleteManagedType(*f_i); - (*record_val)[field] = std::nullopt; + f_i = std::nullopt; Modified(); } diff --git a/src/Val.h b/src/Val.h index 01e4a201ee..9808275072 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1179,9 +1179,10 @@ public: void Assign(int field, StringVal* new_val) { - if ( HasField(field) ) - ZVal::DeleteManagedType(*(*record_val)[field]); - (*record_val)[field] = ZVal(new_val); + auto& fv = (*record_val)[field]; + if ( fv ) + ZVal::DeleteManagedType(*fv); + fv = ZVal(new_val); AddedField(field); } void Assign(int field, const char* new_val) { Assign(field, new StringVal(new_val)); } @@ -1194,7 +1195,7 @@ public: */ template void AssignField(const char* field_name, T&& val) { - int idx = GetType()->AsRecordType()->FieldOffset(field_name); + int idx = rt->FieldOffset(field_name); if ( idx < 0 ) reporter->InternalError("missing record field: %s", field_name); Assign(idx, std::forward(val)); @@ -1212,7 +1213,13 @@ public: * @param field The field index to retrieve. * @return Whether there's a value for the given field index. */ - bool HasField(int field) const { return (*record_val)[field] ? true : false; } + bool HasField(int field) const + { + if ( (*record_val)[field] ) + return true; + + return bool(rt->FieldInits()[field]); + } /** * Returns true if the given field is in the record, false if @@ -1222,7 +1229,7 @@ public: */ bool HasField(const char* field) const { - int idx = GetType()->AsRecordType()->FieldOffset(field); + int idx = rt->FieldOffset(field); return (idx != -1) && HasField(idx); } @@ -1233,10 +1240,17 @@ public: */ ValPtr GetField(int field) const { - if ( ! HasField(field) ) - return nullptr; + auto& fv = (*record_val)[field]; + if ( ! fv ) + { + const auto& fi = rt->FieldInits()[field]; + if ( ! fi ) + return nullptr; - return (*record_val)[field]->ToVal(rt->GetFieldType(field)); + fv = (*fi)->Generate(); + } + + return fv->ToVal(rt->GetFieldType(field)); } /** @@ -1364,7 +1378,7 @@ public: template auto GetFieldAs(const char* field) const { - int idx = GetType()->AsRecordType()->FieldOffset(field); + int idx = rt->FieldOffset(field); if ( idx < 0 ) reporter->InternalError("missing record field: %s", field); @@ -1437,7 +1451,18 @@ protected: // Caller assumes responsibility for memory management. The first // version allows manipulation of whether the field is present at all. // The second version ensures that the optional value is present. - std::optional& RawOptField(int field) { return (*record_val)[field]; } + std::optional& RawOptField(int field) + { + auto& f = (*record_val)[field]; + if ( ! f ) + { + const auto& fi = rt->FieldInits()[field]; + if ( fi ) + f = (*fi)->Generate(); + } + + return f; + } ZVal& RawField(int field) { @@ -1459,8 +1484,9 @@ protected: private: void DeleteFieldIfManaged(unsigned int field) { - if ( HasField(field) && IsManaged(field) ) - ZVal::DeleteManagedType(*(*record_val)[field]); + auto& f = (*record_val)[field]; + if ( f && IsManaged(field) ) + ZVal::DeleteManagedType(*f); } bool IsManaged(unsigned int offset) const { return is_managed[offset]; } diff --git a/src/ZVal.h b/src/ZVal.h index 202659d17b..04a0717bb8 100644 --- a/src/ZVal.h +++ b/src/ZVal.h @@ -10,6 +10,7 @@ namespace zeek { class AddrVal; +class EnumVal; class File; class Func; class ListVal; From ee358affda39924d5e30139e099ad0d751d3b127 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Sat, 15 Apr 2023 20:12:49 -0700 Subject: [PATCH 5/8] clarifications and tidying for record field initializations --- src/Type.cc | 37 +++++++++++++++++++++---------------- src/Type.h | 28 +++++++++++++++------------- src/Val.cc | 2 +- src/Val.h | 6 +++--- 4 files changed, 40 insertions(+), 33 deletions(-) diff --git a/src/Type.cc b/src/Type.cc index e4ac1968a5..b1e1d55351 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -992,6 +992,7 @@ void TypeDecl::DescribeReST(ODesc* d, bool roles_only) const } } +// A record field initialization that directly assigns a fixed value ... class DirectFieldInit final : public FieldInit { public: @@ -1003,6 +1004,7 @@ private: ZVal init_val; }; +// ... the same, but for a value that needs memory management. class DirectManagedFieldInit final : public FieldInit { public: @@ -1019,6 +1021,7 @@ private: ZVal init_val; }; +// A record field initialization that's done by evaluating an expression. class ExprFieldInit final : public FieldInit { public: @@ -1053,6 +1056,8 @@ private: RecordTypePtr coerce_type; // non-nil iff coercion is required }; +// A record field initialization where the field is initialized to an +// empty/default record of the given type. class RecordFieldInit final : public FieldInit { public: @@ -1064,6 +1069,8 @@ private: RecordTypePtr init_type; }; +// A record field initialization where the field is initialized to an +// empty table of the given type. class TableFieldInit final : public FieldInit { public: @@ -1079,6 +1086,8 @@ private: detail::AttributesPtr attrs; }; +// A record field initialization where the field is initialized to an +// empty vector of the given type. class VectorFieldInit final : public FieldInit { public: @@ -1125,26 +1134,22 @@ RecordType::~RecordType() delete types; } - - for ( auto fi : field_inits ) - if ( fi ) - delete *fi; } void RecordType::AddField(unsigned int field, const TypeDecl* td) { - ASSERT(field == field_inits.size()); + ASSERT(field == deferred_inits.size()); ASSERT(field == managed_fields.size()); managed_fields.push_back(ZVal::IsManagedType(td->type)); - // We defer error-checking until here so that we can keep field_inits + // We defer error-checking until here so that we can keep deferred_inits // and managed_fields correctly tracking the associated fields. if ( field_ids.count(td->id) != 0 ) { reporter->Error("duplicate field '%s' found in record definition", td->id); - field_inits.push_back(std::nullopt); + deferred_inits.push_back(std::nullopt); return; } @@ -1156,7 +1161,7 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) auto def_attr = a ? a->Find(detail::ATTR_DEFAULT) : nullptr; auto def_expr = def_attr ? def_attr->GetExpr() : nullptr; - std::optional init; + std::optional> init; if ( def_expr && ! IsErrorType(type->Tag()) ) { @@ -1166,15 +1171,15 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) auto zv = ZVal(v, type); if ( ZVal::IsManagedType(type) ) - init = new DirectManagedFieldInit(zv); + init = std::make_unique(zv); else - init = new DirectFieldInit(zv); + init = std::make_unique(zv); } else { - auto efi = new ExprFieldInit(def_expr, type); - field_expr_inits.emplace_back(std::make_pair(field, efi)); + auto efi = std::make_unique(def_expr, type); + creation_inits.emplace_back(std::make_pair(field, std::move(efi))); } } @@ -1183,16 +1188,16 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) TypeTag tag = type->Tag(); if ( tag == TYPE_RECORD ) - init = new RecordFieldInit(cast_intrusive(type)); + init = std::make_unique(cast_intrusive(type)); else if ( tag == TYPE_TABLE ) - init = new TableFieldInit(cast_intrusive(type), a); + init = std::make_unique(cast_intrusive(type), a); else if ( tag == TYPE_VECTOR ) - init = new VectorFieldInit(cast_intrusive(type)); + init = std::make_unique(cast_intrusive(type)); } - field_inits.push_back(init); + deferred_inits.push_back(std::move(init)); } bool RecordType::HasField(const char* field) const diff --git a/src/Type.h b/src/Type.h index f3edba43fb..afab6ccf1e 100644 --- a/src/Type.h +++ b/src/Type.h @@ -690,9 +690,6 @@ public: void AddFieldsDirectly(const type_decl_list& types, bool add_log_attr = false); - const auto& FieldInits() const { return field_inits; } - const auto& FieldExprInits() const { return field_expr_inits; } - void DescribeReST(ODesc* d, bool roles_only = false) const override; void DescribeFields(ODesc* d) const; void DescribeFieldsReST(ODesc* d, bool func_args) const; @@ -720,17 +717,22 @@ protected: void DoDescribe(ODesc* d) const override; - // Maps each field to how to initialize it. Uses pointers due to - // keeping the FieldInit definition private to Type.cc (see above). - std::vector> field_inits; + // Field initializations that can be deferred to first access, + // beneficial for fields that are separately iniitialized prior + // to first access. + std::vector>> deferred_inits; - // Holds initializations defined in terms of evaluating expressions, - // in pairs (we use pairs instead of a vector - // with per-field expressions because such expressions are not often - // used). These need to be evaluated at record construction time, - // rather than deferring until first use, because the value of the - // expression can change between the two. - std::vector> field_expr_inits; + // Field initializations that need to be done upon record creation, + // rather than deferred. These are expressions whose value might + // change if computed later. + // + // Such initializations are uncommon, so we represent them using + // pairs. + std::vector>> creation_inits; + + friend zeek::RecordVal; + const auto& DeferredInits() const { return deferred_inits; } + const auto& CreationInits() const { return creation_inits; } // If we were willing to bound the size of records, then we could // use std::bitset here instead. diff --git a/src/Val.cc b/src/Val.cc index 7f45e13926..635a022ba1 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -2765,7 +2765,7 @@ RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t), is_managed(t-> { record_val->resize(n); - for ( auto& e : rt->FieldExprInits() ) + for ( auto& e : rt->CreationInits() ) { try { diff --git a/src/Val.h b/src/Val.h index 9808275072..eb2b789bd6 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1218,7 +1218,7 @@ public: if ( (*record_val)[field] ) return true; - return bool(rt->FieldInits()[field]); + return bool(rt->DeferredInits()[field]); } /** @@ -1243,7 +1243,7 @@ public: auto& fv = (*record_val)[field]; if ( ! fv ) { - const auto& fi = rt->FieldInits()[field]; + const auto& fi = rt->DeferredInits()[field]; if ( ! fi ) return nullptr; @@ -1456,7 +1456,7 @@ protected: auto& f = (*record_val)[field]; if ( ! f ) { - const auto& fi = rt->FieldInits()[field]; + const auto& fi = rt->DeferredInits()[field]; if ( fi ) f = (*fi)->Generate(); } From c19eba62d64604af6f86bca3888171a7c3a43347 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Tue, 18 Apr 2023 09:41:45 -0700 Subject: [PATCH 6/8] restored RecordType::Create, now marked as deprecated tidying of namespaces and private class members simplification of flagging record field initializations that should be skipped address peculiar MSVC compilation complaint for IntrusivePtr's --- src/IntrusivePtr.h | 12 +++++++----- src/Type.cc | 33 +++++++++++++++++++++++++-------- src/Type.h | 40 ++++++++++++++++++++++++---------------- src/Val.cc | 8 ++++++++ src/Val.h | 6 +++--- 5 files changed, 67 insertions(+), 32 deletions(-) diff --git a/src/IntrusivePtr.h b/src/IntrusivePtr.h index 7d9578d29f..c94af83d16 100644 --- a/src/IntrusivePtr.h +++ b/src/IntrusivePtr.h @@ -28,10 +28,11 @@ struct NewRef }; /** - * This has to be forward declared and known here in order for us to be able - * cast this in the `Unref` function. + * These have to be forward-declared and known here in order for us to be able + * cast them in the `Unref` function. */ class OpaqueVal; +class TypeVal; /** * An intrusive, reference counting smart pointer implementation. Much like @@ -120,9 +121,10 @@ public: { if ( ptr_ ) { - // Specializing `OpaqueVal` as MSVC compiler does not detect it - // inheriting from `zeek::Obj` so we have to do that manually. - if constexpr ( std::is_same_v ) + // Specializing `Val` subclasses that the MSVC compiler + // does not detect as inheriting from `zeek::Obj` + // so we have to do that manually. + if constexpr ( std::is_same_v || std::is_same_v ) Unref(reinterpret_cast(ptr_)); else Unref(ptr_); diff --git a/src/Type.cc b/src/Type.cc index b1e1d55351..bd03096917 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -992,6 +992,9 @@ void TypeDecl::DescribeReST(ODesc* d, bool roles_only) const } } +namespace detail + { + // A record field initialization that directly assigns a fixed value ... class DirectFieldInit final : public FieldInit { @@ -1099,6 +1102,8 @@ private: VectorTypePtr init_type; }; + } // namespace detail + RecordType::RecordType(type_decl_list* arg_types) : Type(TYPE_RECORD) { types = arg_types; @@ -1149,7 +1154,7 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) if ( field_ids.count(td->id) != 0 ) { reporter->Error("duplicate field '%s' found in record definition", td->id); - deferred_inits.push_back(std::nullopt); + deferred_inits.push_back(nullptr); return; } @@ -1161,7 +1166,7 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) auto def_attr = a ? a->Find(detail::ATTR_DEFAULT) : nullptr; auto def_expr = def_attr ? def_attr->GetExpr() : nullptr; - std::optional> init; + std::unique_ptr init; if ( def_expr && ! IsErrorType(type->Tag()) ) { @@ -1171,14 +1176,14 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) auto zv = ZVal(v, type); if ( ZVal::IsManagedType(type) ) - init = std::make_unique(zv); + init = std::make_unique(zv); else - init = std::make_unique(zv); + init = std::make_unique(zv); } else { - auto efi = std::make_unique(def_expr, type); + auto efi = std::make_unique(def_expr, type); creation_inits.emplace_back(std::make_pair(field, std::move(efi))); } } @@ -1188,13 +1193,13 @@ void RecordType::AddField(unsigned int field, const TypeDecl* td) TypeTag tag = type->Tag(); if ( tag == TYPE_RECORD ) - init = std::make_unique(cast_intrusive(type)); + init = std::make_unique(cast_intrusive(type)); else if ( tag == TYPE_TABLE ) - init = std::make_unique(cast_intrusive(type), a); + init = std::make_unique(cast_intrusive(type), a); else if ( tag == TYPE_VECTOR ) - init = std::make_unique(cast_intrusive(type)); + init = std::make_unique(cast_intrusive(type)); } deferred_inits.push_back(std::move(init)); @@ -1392,6 +1397,18 @@ void RecordType::AddFieldsDirectly(const type_decl_list& others, bool add_log_at num_fields = types->length(); } +void RecordType::Create(std::vector>& r) const + { + for ( auto& di : deferred_inits ) + if ( di ) + r.push_back(di->Generate()); + else + r.push_back(std::nullopt); + + for ( auto& ci : creation_inits ) + r[ci.first] = ci.second->Generate(); + } + void RecordType::DescribeFields(ODesc* d) const { if ( d->IsReadable() ) diff --git a/src/Type.h b/src/Type.h index afab6ccf1e..6aa43b31b9 100644 --- a/src/Type.h +++ b/src/Type.h @@ -37,6 +37,16 @@ class ListExpr; class Attributes; using ListExprPtr = IntrusivePtr; +// The following tracks how to initialize a given record field. +class FieldInit + { +public: + virtual ~FieldInit() { } + + // Return the initialization value of the field. + virtual ZVal Generate() const = 0; + }; + } // namespace detail // Zeek types. @@ -601,17 +611,6 @@ public: using type_decl_list = PList; -// The following tracks how to initialize a given field. - -class FieldInit - { -public: - virtual ~FieldInit() { } - - // Return the initialization value of the field. - virtual ZVal Generate() const = 0; - }; - class RecordType final : public Type { public: @@ -690,6 +689,15 @@ public: void AddFieldsDirectly(const type_decl_list& types, bool add_log_attr = false); + /** + * + * Populates a new instance of the record with its initial values. + * @param r The record's underlying value vector. + */ + [[deprecated("Remove in v6.1. Construct a corresponding RecordVal and build vector from " + "GetFieldAs() calls.")]] void + Create(std::vector>& r) const; + void DescribeReST(ODesc* d, bool roles_only = false) const override; void DescribeFields(ODesc* d) const; void DescribeFieldsReST(ODesc* d, bool func_args) const; @@ -710,7 +718,7 @@ public: detail::TraversalCode Traverse(detail::TraversalCallback* cb) const override; -protected: +private: RecordType() { types = nullptr; } void AddField(unsigned int field, const TypeDecl* td); @@ -718,9 +726,9 @@ protected: void DoDescribe(ODesc* d) const override; // Field initializations that can be deferred to first access, - // beneficial for fields that are separately iniitialized prior - // to first access. - std::vector>> deferred_inits; + // beneficial for fields that are separately initialized prior + // to first access. Nil pointers mean "skip initializing the field". + std::vector> deferred_inits; // Field initializations that need to be done upon record creation, // rather than deferred. These are expressions whose value might @@ -728,7 +736,7 @@ protected: // // Such initializations are uncommon, so we represent them using // pairs. - std::vector>> creation_inits; + std::vector>> creation_inits; friend zeek::RecordVal; const auto& DeferredInits() const { return deferred_inits; } diff --git a/src/Val.cc b/src/Val.cc index 635a022ba1..6218beef98 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -2778,6 +2778,14 @@ RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t), is_managed(t-> throw; } } + + // The following is just for testing the deprecated Create() + // call, and can be removed with 6.1. We leave it commented + // out so we don't get deprecation warnings when building. +#if 0 + std::vector> testing; + rt->Create(testing); +#endif } else diff --git a/src/Val.h b/src/Val.h index eb2b789bd6..7290a6eacd 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1218,7 +1218,7 @@ public: if ( (*record_val)[field] ) return true; - return bool(rt->DeferredInits()[field]); + return rt->DeferredInits()[field] != nullptr; } /** @@ -1247,7 +1247,7 @@ public: if ( ! fi ) return nullptr; - fv = (*fi)->Generate(); + fv = fi->Generate(); } return fv->ToVal(rt->GetFieldType(field)); @@ -1458,7 +1458,7 @@ protected: { const auto& fi = rt->DeferredInits()[field]; if ( fi ) - f = (*fi)->Generate(); + f = fi->Generate(); } return f; From 1f61afa56b0a7281704f47167a1ffe2b6ff20b24 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Tue, 18 Apr 2023 15:48:22 -0700 Subject: [PATCH 7/8] more general approach for addressing MSVC compiler issues with IntrusivePtr --- src/IntrusivePtr.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/IntrusivePtr.h b/src/IntrusivePtr.h index c94af83d16..600bb5a263 100644 --- a/src/IntrusivePtr.h +++ b/src/IntrusivePtr.h @@ -121,10 +121,10 @@ public: { if ( ptr_ ) { - // Specializing `Val` subclasses that the MSVC compiler - // does not detect as inheriting from `zeek::Obj` + // For some `Val` subclasses the MSVC compiler does + // not detect them as inheriting from `zeek::Obj`, // so we have to do that manually. - if constexpr ( std::is_same_v || std::is_same_v ) + if constexpr ( std::is_base_of_v ) Unref(reinterpret_cast(ptr_)); else Unref(ptr_); From 11dc4c8ce9a933f61753d31c772bd7e5ef833883 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Tue, 18 Apr 2023 17:12:52 -0700 Subject: [PATCH 8/8] different fix for MSVC compiler issues --- src/IntrusivePtr.h | 12 +++++------- src/Type.h | 2 +- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/IntrusivePtr.h b/src/IntrusivePtr.h index 600bb5a263..7d9578d29f 100644 --- a/src/IntrusivePtr.h +++ b/src/IntrusivePtr.h @@ -28,11 +28,10 @@ struct NewRef }; /** - * These have to be forward-declared and known here in order for us to be able - * cast them in the `Unref` function. + * This has to be forward declared and known here in order for us to be able + * cast this in the `Unref` function. */ class OpaqueVal; -class TypeVal; /** * An intrusive, reference counting smart pointer implementation. Much like @@ -121,10 +120,9 @@ public: { if ( ptr_ ) { - // For some `Val` subclasses the MSVC compiler does - // not detect them as inheriting from `zeek::Obj`, - // so we have to do that manually. - if constexpr ( std::is_base_of_v ) + // Specializing `OpaqueVal` as MSVC compiler does not detect it + // inheriting from `zeek::Obj` so we have to do that manually. + if constexpr ( std::is_same_v ) Unref(reinterpret_cast(ptr_)); else Unref(ptr_); diff --git a/src/Type.h b/src/Type.h index 6aa43b31b9..311788daa7 100644 --- a/src/Type.h +++ b/src/Type.h @@ -15,13 +15,13 @@ #include "zeek/IntrusivePtr.h" #include "zeek/Obj.h" #include "zeek/Traverse.h" -#include "zeek/ZVal.h" #include "zeek/ZeekList.h" namespace zeek { class Val; +union ZVal; class EnumVal; class RecordVal; class TableVal;