[gjs: 1/2] object: Use std::vector to hold Objects GClosure's

[Philip Chimento <pchimento src gnome org>]

commit 203ff94d4797121a1ac96d04e5bb766f06207e83
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Fri Jun 10 03:11:00 2022 +0200

    object: Use std::vector to hold Objects GClosure's
    
    We used a std::forward list to hold closure pointers in Object's to
    avoid adding a vector when not needed, but these structures are terribly
    slow as already pointed out in [1], and demonstrated even way poorer
    performances when testing signal connections (see [2], #485 and !758), so get
    rid of them and just use vectors everywhere which can boost performances
    up to 95% in our tests.
    
    This implies some more memory usage for each object, as we can't use a
    std::vector without the size member since C++11, but I think it's still
    better, given that size of std::forward_list is still bigger when with
    only two closures/vfuncs added (as each element requires two pointers).
    
    Fixes: #485
    
    [1] https://github.com/3v1n0/stl-containers-benchmarks
    [2] https://gitlab.gnome.org/3v1n0/gjs/-/snippets/3608

 gi/object.cpp  | 24 ++++++++++++------------
 gi/object.h    |  5 ++---
 gjs/gjs_pch.hh |  1 -
 3 files changed, 14 insertions(+), 16 deletions(-)
---
diff --git a/gi/object.cpp b/gi/object.cpp
index 6c85631c9..a9e39fd27 100644
--- a/gi/object.cpp
+++ b/gi/object.cpp
@@ -78,7 +78,7 @@ class JSTracer;
 #if defined(__x86_64__) && defined(__clang__)
 /* This isn't meant to be comprehensive, but should trip on at least one CI job
  * if sizeof(ObjectInstance) is increased. */
-static_assert(sizeof(ObjectInstance) <= 48,
+static_assert(sizeof(ObjectInstance) <= 64,
               "Think very hard before increasing the size of ObjectInstance. "
               "There can be tens of thousands of them alive in a typical "
               "gnome-shell run.");
@@ -1697,19 +1697,18 @@ bool ObjectInstance::ensure_uses_toggle_ref(JSContext* cx) {
     return true;
 }
 
-static void invalidate_closure_list(std::forward_list<GClosure*>* closures,
-                                    void* data, GClosureNotify notify_func) {
+static void invalidate_closure_vector(std::vector<GClosure*>* closures,
+                                      void* data, GClosureNotify notify_func) {
     g_assert(closures);
     g_assert(notify_func);
 
-    auto before = closures->before_begin();
     for (auto it = closures->begin(); it != closures->end();) {
         // This will also free the closure data, through the closure
         // invalidation mechanism, but adding a temporary reference to
         // ensure that the closure is still valid when calling invalidation
         // notify callbacks
-        GjsAutoGClosure closure(closures->front(), GjsAutoTakeOwnership());
-        it = closures->erase_after(before);
+        GjsAutoGClosure closure(*it, GjsAutoTakeOwnership());
+        it = closures->erase(it);
 
         // Only call the invalidate notifiers that won't touch this vector
         g_closure_remove_invalidate_notifier(closure, data, notify_func);
@@ -1967,7 +1966,7 @@ ObjectInstance::~ObjectInstance() {
 }
 
 ObjectPrototype::~ObjectPrototype() {
-    invalidate_closure_list(&m_vfuncs, this, &vfunc_invalidated_notify);
+    invalidate_closure_vector(&m_vfuncs, this, &vfunc_invalidated_notify);
 
     g_type_class_unref(g_type_class_peek(m_gtype));
 
@@ -2104,7 +2103,7 @@ bool ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
 
     /* This is a weak reference, and will be cleared when the closure is
      * invalidated */
-    m_closures.push_front(closure);
+    m_closures.push_back(closure);
     g_closure_add_invalidate_notifier(
         closure, this, &ObjectInstance::closure_invalidated_notify);
 
@@ -2114,11 +2113,12 @@ bool ObjectInstance::associate_closure(JSContext* cx, GClosure* closure) {
 void ObjectInstance::closure_invalidated_notify(void* data, GClosure* closure) {
     // This callback should *only* touch m_closures
     auto* priv = static_cast<ObjectInstance*>(data);
-    priv->m_closures.remove(closure);
+    Gjs::remove_one_from_unsorted_vector(&priv->m_closures, closure);
 }
 
 void ObjectInstance::invalidate_closures() {
-    invalidate_closure_list(&m_closures, this, &closure_invalidated_notify);
+    invalidate_closure_vector(&m_closures, this, &closure_invalidated_notify);
+    m_closures.shrink_to_fit();
 }
 
 bool ObjectBase::connect(JSContext* cx, unsigned argc, JS::Value* vp) {
@@ -3024,7 +3024,7 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
         g_assert(std::find(m_vfuncs.begin(), m_vfuncs.end(), trampoline) ==
                      m_vfuncs.end() &&
                  "This vfunc was already associated with this class");
-        m_vfuncs.push_front(trampoline);
+        m_vfuncs.push_back(trampoline);
         g_closure_add_invalidate_notifier(
             trampoline, this, &ObjectPrototype::vfunc_invalidated_notify);
         g_closure_add_invalidate_notifier(
@@ -3040,7 +3040,7 @@ bool ObjectPrototype::hook_up_vfunc_impl(JSContext* cx,
 void ObjectPrototype::vfunc_invalidated_notify(void* data, GClosure* closure) {
     // This callback should *only* touch m_vfuncs
     auto* priv = static_cast<ObjectPrototype*>(data);
-    priv->m_vfuncs.remove(closure);
+    Gjs::remove_one_from_unsorted_vector(&priv->m_vfuncs, closure);
 }
 
 bool
diff --git a/gi/object.h b/gi/object.h
index 886f13eaf..f73e73585 100644
--- a/gi/object.h
+++ b/gi/object.h
@@ -10,7 +10,6 @@
 #include <stddef.h>  // for size_t
 #include <stdint.h>  // for uint32_t
 
-#include <forward_list>
 #include <functional>
 #include <vector>
 
@@ -197,7 +196,7 @@ class ObjectPrototype
     FieldCache m_field_cache;
     NegativeLookupCache m_unresolvable_cache;
     // a list of vfunc GClosures installed on this prototype, used when tracing
-    std::forward_list<GClosure*> m_vfuncs;
+    std::vector<GClosure*> m_vfuncs;
     // a list of interface types explicitly associated with this prototype,
     // by gjs_add_interface
     std::vector<GType> m_interface_gtypes;
@@ -295,7 +294,7 @@ class ObjectInstance : public GIWrapperInstance<ObjectBase, ObjectPrototype,
     GjsMaybeOwned<JSObject*> m_wrapper;
     // a list of all GClosures installed on this object (from signal connections
     // and scope-notify callbacks passed to methods), used when tracing
-    std::forward_list<GClosure*> m_closures;
+    std::vector<GClosure*> m_closures;
 
     bool m_wrapper_finalized : 1;
     bool m_gobj_disposed : 1;
diff --git a/gjs/gjs_pch.hh b/gjs/gjs_pch.hh
index 6f1b0b4fb..337e585f5 100644
--- a/gjs/gjs_pch.hh
+++ b/gjs/gjs_pch.hh
@@ -10,7 +10,6 @@
 #include <cmath>
 #include <cstddef>
 #include <deque>
-#include <forward_list>
 #include <functional>
 #include <limits>
 #include <memory>