From 062a1ee6b37b7686b7eea7c455901797316a937d Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 6 Jun 2019 23:13:43 +0000 Subject: [PATCH] Redo API for notifiers. There's now an notifier::Modifiable interface class that class supposed to signal modifications are to be derived from. This takes the place of the former MutableValue class and also unifies how Val and IDs signal modifications. --- src/ID.cc | 2 +- src/ID.h | 2 +- src/StateAccess.cc | 99 +++++++++-------------------------- src/StateAccess.h | 125 ++++++++++++++++++++++++++------------------- src/Trigger.cc | 30 ++++------- src/Trigger.h | 9 ++-- src/Val.cc | 6 +-- src/Val.h | 16 ++++-- 8 files changed, 126 insertions(+), 163 deletions(-) diff --git a/src/ID.cc b/src/ID.cc index 582c8fcf8e..894de949a5 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -66,7 +66,7 @@ void ID::SetVal(Val* v, bool arg_weak_ref) val = v; weak_ref = arg_weak_ref; - notifiers.Modified(this); + Modified(); #ifdef DEBUG UpdateValID(); diff --git a/src/ID.h b/src/ID.h index 7717a584f3..a1cbe494d3 100644 --- a/src/ID.h +++ b/src/ID.h @@ -15,7 +15,7 @@ class Func; typedef enum { INIT_NONE, INIT_FULL, INIT_EXTRA, INIT_REMOVE, } init_class; typedef enum { SCOPE_FUNCTION, SCOPE_MODULE, SCOPE_GLOBAL } IDScope; -class ID : public BroObj { +class ID : public BroObj, public notifier::Modifiable { public: ID(const char* name, IDScope arg_scope, bool arg_is_export); ~ID() override; diff --git a/src/StateAccess.cc b/src/StateAccess.cc index 853a8a7879..7d7c397981 100644 --- a/src/StateAccess.cc +++ b/src/StateAccess.cc @@ -4,110 +4,57 @@ #include "NetVar.h" #include "DebugLogger.h" -NotifierRegistry notifiers; +notifier::Registry notifier::registry; -NotifierRegistry::~NotifierRegistry() +notifier::Registry::~Registry() { - for ( auto i : ids ) - Unref(i.first); - - for ( auto i : vals ) - Unref(i.first); + for ( auto i : registrations ) + Unregister(i.first); } -void NotifierRegistry::Register(ID* id, NotifierRegistry::Notifier* notifier) +void notifier::Registry::Register(Modifiable* m, notifier::Notifier* notifier) { - DBG_LOG(DBG_NOTIFIERS, "registering ID %s for notifier %s", - id->Name(), notifier->Name()); + DBG_LOG(DBG_NOTIFIERS, "registering modifiable %p for notifier %s", + m, notifier->Name()); - Attr* attr = new Attr(ATTR_TRACKED); - - if ( id->Attrs() ) - { - if ( ! id->Attrs()->FindAttr(ATTR_TRACKED) ) - id->Attrs()->AddAttr(attr); - } - else - { - attr_list* a = new attr_list{attr}; - id->SetAttrs(new Attributes(a, id->Type(), false)); - } - - Unref(attr); - - ids.insert({id, notifier}); - Ref(id); + registrations.insert({m, notifier}); + ++m->notifiers; } -void NotifierRegistry::Register(Val* val, NotifierRegistry::Notifier* notifier) +void notifier::Registry::Unregister(Modifiable* m, notifier::Notifier* notifier) { - if ( ! val->IsMutableVal() ) - return; + DBG_LOG(DBG_NOTIFIERS, "unregistering modifiable %p from notifier %s", + m, notifier->Name()); - DBG_LOG(DBG_NOTIFIERS, "registering value %p for notifier %s", - val, notifier->Name()); - - vals.insert({val, notifier}); - Ref(val); - } - -void NotifierRegistry::Unregister(ID* id, NotifierRegistry::Notifier* notifier) - { - DBG_LOG(DBG_NOTIFIERS, "unregistering ID %s for notifier %s", - id->Name(), notifier->Name()); - - auto x = ids.equal_range(id); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) { if ( i->second == notifier ) { - ids.erase(i); - Unref(id); - break; - } - } - - if ( ids.find(id) == ids.end() ) - // Last registered notifier for this ID - id->Attrs()->RemoveAttr(ATTR_TRACKED); - } - -void NotifierRegistry::Unregister(Val* val, NotifierRegistry::Notifier* notifier) - { - DBG_LOG(DBG_NOTIFIERS, "unregistering Val %p for notifier %s", - val, notifier->Name()); - - auto x = vals.equal_range(val); - for ( auto i = x.first; i != x.second; i++ ) - { - if ( i->second == notifier ) - { - vals.erase(i); - Unref(val); + registrations.erase(i); + --i->first->notifiers; break; } } } -void NotifierRegistry::Modified(Val *val) +void notifier::Registry::Unregister(Modifiable* m) { - DBG_LOG(DBG_NOTIFIERS, "modification to tracked value %p", val); - - auto x = vals.equal_range(val); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) - i->second->Modified(val); + Unregister(m, i->second); } -void NotifierRegistry::Modified(ID *id) +void notifier::Registry::Modified(Modifiable* m) { - DBG_LOG(DBG_NOTIFIERS, "modification to tracked ID %s", id->Name()); + DBG_LOG(DBG_NOTIFIERS, "modification to modifiable %p", m); - auto x = ids.equal_range(id); + auto x = registrations.equal_range(m); for ( auto i = x.first; i != x.second; i++ ) - i->second->Modified(id); + i->second->Modified(m); } -const char* NotifierRegistry::Notifier::Name() const +const char* notifier::Notifier::Name() const { return fmt("%p", this); } diff --git a/src/StateAccess.h b/src/StateAccess.h index 528e3357d5..91a7fa6186 100644 --- a/src/StateAccess.h +++ b/src/StateAccess.h @@ -1,4 +1,14 @@ // A class describing a state-modyfing access to a Value or an ID. +// +// TODO UPDATE: We provide a notifier framework to inform interested parties +// of modifications to selected global IDs/Vals. To get notified about a +// change, derive a class from Notifier and register the interesting +// instances with the NotifierRegistry. +// +// Note: For containers (e.g., tables), notifications are only issued if the +// container itself is modified, *not* for changes to the values contained +// therein. + #ifndef STATEACESSS_H #define STATEACESSS_H @@ -7,63 +17,74 @@ #include #include -class Val; -class ID; -class MutableVal; -class HashKey; -class ODesc; -class TableVal; +#include "util.h" -// We provide a notifier framework to inform interested parties of -// modifications to selected global IDs/Vals. To get notified about a change, -// derive a class from Notifier and register the interesting IDs/Vals with -// the NotifierRegistry. -// -// Note: For containers (e.g., tables), notifications are only issued if the -// container itself is modified, *not* for changes to the values contained -// therein. +namespace notifier { -class NotifierRegistry { +class Modifiable; + +class Notifier { public: - class Notifier { - public: - virtual ~Notifier() { } + virtual ~Notifier() { } - // Called when a change is being performed. Note that when - // these methods are called, it is undefined whether the - // change has already been done or is just going to be - // performed soon. - virtual void Modified(ID* id) = 0; - virtual void Modified(Val* val) = 0; - virtual const char* Name() const; // for debugging - }; - - NotifierRegistry() { } - ~NotifierRegistry(); - - // Register a new notifier to be informed when ID/Val changes. Note - // that the registry will store a reference to the target, keeping - // the instance alive for as long as it's registered. - void Register(ID* id, Notifier* notifier); - void Register(Val* val, Notifier* notifier); - - // Cancel a notifier's tracking for this ID/Val, also releasing the - // referencee being held. - void Unregister(ID* id, Notifier* notifier); - void Unregister(Val* val, Notifier* notifier); - - // Inform all registered notifiiers of a modification to a value/ID. - void Modified(ID *id); - void Modified(Val *val); - -private: - typedef std::unordered_multimap ValMap; - typedef std::unordered_multimap IDMap; - - ValMap vals; - IDMap ids; + // Called afger a change has been performed. + virtual void Modified(Modifiable* m) = 0; + virtual const char* Name() const; // for debugging }; -extern NotifierRegistry notifiers; +// Singleton class. +class Registry { +public: + Registry() { } + ~Registry(); + + // Register a new notifier to be informed when an instance changes. + void Register(Modifiable* m, Notifier* notifier); + + // Cancel a notifier's tracking an instace. + void Unregister(Modifiable* m, Notifier* notifier); + + // Cancel all notifiers registered for an instance. + void Unregister(Modifiable* m); + +private: + friend class Modifiable; + + // Inform all registered notifiers of a modification to an instance. + void Modified(Modifiable* m); + + typedef std::unordered_multimap ModifiableMap; + ModifiableMap registrations; +}; + + +class Registry; +extern Registry registry; + +// Base class for objects wanting to signal modifications to the registry. +class Modifiable { +protected: + friend class Registry; + + Modifiable() {} + ~Modifiable() + { + if ( notifiers ) + registry.Unregister(this); + + }; + + void Modified() + { + if ( notifiers ) + registry.Modified(this); + } + + // Number of currently registered notifiers for this instance. + uint64 notifiers; +}; + +} + #endif diff --git a/src/Trigger.cc b/src/Trigger.cc index 213707b6b8..c1295b8218 100644 --- a/src/Trigger.cc +++ b/src/Trigger.cc @@ -33,7 +33,7 @@ TraversalCode TriggerTraversalCallback::PreExpr(const Expr* expr) trigger->Register(e->Id()); Val* v = e->Id()->ID_Val(); - if ( v && v->IsMutableVal() ) + if ( v && v->Modifiable() ) trigger->Register(v); break; }; @@ -382,38 +382,30 @@ void Trigger::Timeout() void Trigger::Register(ID* id) { assert(! disabled); - notifiers.Register(id, this); + notifier::registry.Register(id, this); Ref(id); - ids.insert(id); + objs.push_back(id); } void Trigger::Register(Val* val) { + if ( ! val->Modifiable() ) + return; + assert(! disabled); - notifiers.Register(val, this); + notifier::registry.Register(val->Modifiable(), this); Ref(val); - vals.insert(val); + objs.push_back(val); } void Trigger::UnregisterAll() { - loop_over_list(ids, i) - { - notifiers.Unregister(ids[i], this); - Unref(ids[i]); - } + for ( auto o : objs ) + Unref(o); // this will unregister with the registry as well - ids.clear(); - - loop_over_list(vals, j) - { - notifiers.Unregister(vals[j], this); - Unref(vals[j]); - } - - vals.clear(); + objs.clear(); } void Trigger::Attach(Trigger *trigger) diff --git a/src/Trigger.h b/src/Trigger.h index 8dc478e049..fe7d425a72 100644 --- a/src/Trigger.h +++ b/src/Trigger.h @@ -13,7 +13,7 @@ class TriggerTimer; class TriggerTraversalCallback; -class Trigger : public NotifierRegistry::Notifier, public BroObj { +class Trigger : public notifier::Notifier, public BroObj { public: // Don't access Trigger objects; they take care of themselves after // instantiation. Note that if the condition is already true, the @@ -61,9 +61,7 @@ public: { d->Add(""); } // Overidden from Notifier. We queue the trigger and evaluate it // later to avoid race conditions. - void Modified(ID* id) override - { QueueTrigger(this); } - void Modified(Val* val) override + void Modified(notifier::Modifiable* m) override { QueueTrigger(this); } const char* Name() const override; @@ -104,8 +102,7 @@ private: bool delayed; // true if a function call is currently being delayed bool disabled; - val_list vals; - id_list ids; + std::vector objs; typedef map ValCache; ValCache cache; diff --git a/src/Val.cc b/src/Val.cc index 4ba2ea6dd7..bea9ce4038 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -899,7 +899,7 @@ static void table_entry_val_delete_func(void* val) delete tv; } -TableVal::TableVal(TableType* t, Attributes* a) : MutableVal(t) +TableVal::TableVal(TableType* t, Attributes* a) : Val(t) { Init(t); SetAttrs(a); @@ -1982,7 +1982,7 @@ unsigned int TableVal::MemoryAllocation() const vector RecordVal::parse_time_records; -RecordVal::RecordVal(RecordType* t, bool init_fields) : MutableVal(t) +RecordVal::RecordVal(RecordType* t, bool init_fields) : Val(t) { origin = 0; record_type = t; @@ -2287,7 +2287,7 @@ Val* EnumVal::DoClone(CloneState* state) return Ref(); } -VectorVal::VectorVal(VectorType* t) : MutableVal(t) +VectorVal::VectorVal(VectorType* t) : Val(t) { vector_type = t->Ref()->AsVectorType(); val.vector_val = new vector(); diff --git a/src/Val.h b/src/Val.h index 48f456fa17..d98c352f6d 100644 --- a/src/Val.h +++ b/src/Val.h @@ -328,6 +328,8 @@ public: void Describe(ODesc* d) const override; virtual void DescribeReST(ODesc* d) const; + virtual notifier::Modifiable* Modifiable() { return 0; } + #ifdef DEBUG // For debugging, we keep a reference to the global ID to which a // value has been bound *last*. @@ -490,8 +492,6 @@ protected: MutableVal() {} ~MutableVal() override; - - void Modified() { notifiers.Modified(this); } }; #define Microseconds 1e-6 @@ -764,7 +764,7 @@ protected: }; class CompositeHash; -class TableVal : public MutableVal { +class TableVal : public Val, public notifier::Modifiable { public: explicit TableVal(TableType* t, Attributes* attrs = 0); ~TableVal() override; @@ -877,6 +877,8 @@ public: HashKey* ComputeHash(const Val* index) const { return table_hash->ComputeHash(index, 1); } + notifier::Modifiable* Modifiable() override { return this; } + protected: friend class Val; TableVal() {} @@ -915,7 +917,7 @@ protected: Val* def_val; }; -class RecordVal : public MutableVal { +class RecordVal : public Val, public notifier::Modifiable { public: explicit RecordVal(RecordType* t, bool init_fields = true); ~RecordVal() override; @@ -962,6 +964,8 @@ public: unsigned int MemoryAllocation() const override; void DescribeReST(ODesc* d) const override; + notifier::Modifiable* Modifiable() override { return this; } + // Extend the underlying arrays of record instances created during // parsing to match the number of fields in the record type (they may // mismatch as a result of parse-time record type redefinitions. @@ -1006,7 +1010,7 @@ protected: }; -class VectorVal : public MutableVal { +class VectorVal : public Val, public notifier::Modifiable { public: explicit VectorVal(VectorType* t); ~VectorVal() override; @@ -1054,6 +1058,8 @@ public: // Won't shrink size. unsigned int ResizeAtLeast(unsigned int new_num_elements); + notifier::Modifiable* Modifiable() override { return this; } + protected: friend class Val; VectorVal() { }