From 75e56bb6af07c63d440b217bdc6f8582ab7b7f00 Mon Sep 17 00:00:00 2001 From: Matthew Fernandez Date: Sun, 16 Aug 2026 15:05:36 +1000 Subject: [PATCH] API BREAK: reduce allocations during symbol resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Profiling reveals runtime of `murphi2xml` on the CMurphi example sci.m is dominated by symbol resolution, bottoming out on time spent cloning AST nodes. This change alleviates some of this copying, though more could be done. This change is a 17% speed up on this `murphi2xml` workload, 15.72s → 12.30s. Github: #335 “'std::move' optimisations” --- librumur/include/rumur/Symtab.h | 14 ++++++-- librumur/src/resolve-symbols.cc | 64 +++++++++++++++++++++++---------- 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/librumur/include/rumur/Symtab.h b/librumur/include/rumur/Symtab.h index e0ea252b..d58d43aa 100644 --- a/librumur/include/rumur/Symtab.h +++ b/librumur/include/rumur/Symtab.h @@ -20,7 +20,7 @@ namespace rumur { class RUMUR_API Symtab { private: - std::vector>> scope; + std::vector> scope; public: void open_scope() { scope.emplace_back(); } @@ -30,8 +30,16 @@ class RUMUR_API Symtab { scope.pop_back(); } - void declare(const std::string &name, const Ptr &value) { + /// make a new symbol available for lookup + /// + /// It is assumed `value` will outlive `*this`, and thus can be retained + /// internally. + /// + /// @param name Symbol name + /// @param value Node this name should resolve to + void declare(const std::string &name, const Node *value) { assert(!scope.empty()); + assert(value != nullptr); if (scope.back().count(name) > 0) throw Error("symbol \"" + name + "\" was previously declared", value->loc); @@ -43,7 +51,7 @@ class RUMUR_API Symtab { for (auto it = scope.rbegin(); it != scope.rend(); it++) { auto it2 = it->find(name); if (it2 != it->end()) { - if (auto ret = dynamic_cast(it2->second.get())) { + if (auto ret = dynamic_cast(it2->second)) { return Ptr(ret->clone()); } else { break; diff --git a/librumur/src/resolve-symbols.cc b/librumur/src/resolve-symbols.cc index def7c123..6c243b71 100644 --- a/librumur/src/resolve-symbols.cc +++ b/librumur/src/resolve-symbols.cc @@ -18,11 +18,24 @@ #include #include #include +#include #include #include using namespace rumur; +/// C++11 polyfill for `std::make_unique` +template +static std::unique_ptr make_unique(Args &&...args) { + auto raw = new T(std::forward(args)...); + try { + return std::unique_ptr{raw}; + } catch (std::exception &) { + delete raw; + throw; + } +} + namespace { class Resolver : public Traversal { @@ -30,6 +43,21 @@ class Resolver : public Traversal { private: Symtab symtab; + /// node copies we allocated that we are responsible for cleaning up + /// + /// We can use `std::unique_ptr` instead of `Ptr` here because we never need + /// to copy or subclass these pointers. + std::vector> heap; + + /// create a new managed node pointer + template T *make(Args &&...args) { + std::unique_ptr ptr = make_unique(std::forward(args)...); + heap.push_back(std::move(ptr)); + auto ret = dynamic_cast(heap.back().get()); + assert(ret != nullptr); + return ret; + } + public: Resolver() { @@ -37,13 +65,13 @@ class Resolver : public Traversal { symtab.open_scope(); // Teach the symbol table the built ins - auto td = Ptr::make("boolean", Boolean, location()); - symtab.declare("boolean", td); + TypeDecl *const boolean = make("boolean", Boolean, location()); + symtab.declare("boolean", boolean); mpz_class index = 0; for (const std::pair &m : Boolean->members) { - symtab.declare(m.first, Ptr::make( - m.first, Ptr::make(index, location()), - Boolean, location())); + ConstDecl *const member = make( + m.first, Ptr::make(index, location()), Boolean, location()); + symtab.declare(m.first, member); index++; } } @@ -59,7 +87,7 @@ class Resolver : public Traversal { symtab.open_scope(); for (auto &a : n.aliases) { dispatch(*a); - symtab.declare(a->name, a); + symtab.declare(a->name, a.get()); } for (auto &r : n.rules) dispatch(*r); @@ -70,7 +98,7 @@ class Resolver : public Traversal { symtab.open_scope(); for (auto &a : n.aliases) { dispatch(*a); - symtab.declare(a->name, a); + symtab.declare(a->name, a.get()); } for (auto &s : n.body) dispatch(*s); @@ -122,13 +150,13 @@ class Resolver : public Traversal { mpz_class index = 0; size_t id = e->unique_id + 1; for (const std::pair &m : n.members) { - auto cd = Ptr::make( + ConstDecl *const member = make( m.first, Ptr::make(index, m.second), e, m.second); // assign this member a unique id so that referrers can use it if need be assert(id < e->unique_id_limit && "number of enum members exceeds what was expected"); - cd->unique_id = id; - symtab.declare(m.first, cd); + member->unique_id = id; + symtab.declare(m.first, member); index++; id++; } @@ -186,15 +214,15 @@ class Resolver : public Traversal { // register the function itself, even though its body has not yet been // resolved, in order to allow contained function calls to resolve to the // containing function, supporting recursion - symtab.declare(n.name, Ptr::make(n)); + symtab.declare(n.name, &n); // only register the function parameters now, to avoid their names shadowing // anything that needs to be resolved during symbol resolution of another // parameter or the return type for (auto &p : n.parameters) - symtab.declare(p->name, p); + symtab.declare(p->name, p.get()); for (auto &d : n.decls) { dispatch(*d); - symtab.declare(d->name, d); + symtab.declare(d->name, d.get()); } for (auto &s : n.body) dispatch(*s); @@ -287,9 +315,9 @@ class Resolver : public Traversal { } if (auto d = dynamic_cast(c.get())) - symtab.declare(d->name, c); + symtab.declare(d->name, c.get()); if (auto f = dynamic_cast(c.get())) - symtab.declare(f->name, c); + symtab.declare(f->name, c.get()); } } @@ -358,7 +386,7 @@ class Resolver : public Traversal { dispatch(*n.decl); - symtab.declare(n.name, n.decl); + symtab.declare(n.name, n.decl.get()); } void visit_range(Range &n) final { @@ -399,7 +427,7 @@ class Resolver : public Traversal { dispatch(*n.guard); for (auto &d : n.decls) { dispatch(*d); - symtab.declare(d->name, d); + symtab.declare(d->name, d.get()); } for (auto &s : n.body) dispatch(*s); @@ -414,7 +442,7 @@ class Resolver : public Traversal { dispatch(q); for (auto &d : n.decls) { dispatch(*d); - symtab.declare(d->name, d); + symtab.declare(d->name, d.get()); } for (auto &s : n.body) dispatch(*s);