instanceVar == instanceVar2 does not invoke metamethod _cmp

[rajkosto]

i have 2 instances of same class and i've defined _cmp methamethod on it's type but it's not called with operators == and !=, i have to do something like if ((actualTypeId <=> GOTypeIDs.CodeProxy) == 0) which calls _cmp, while if (actualTypeId == GOTypeIDs.CodeProxy) doesnt

non-intuitive behaviour, please fix

[albertodemichelis]

Implementing this would have a very large impact to performance. I'm afraid is not going to change.

[obfuscated]

@albertodemichelis Do you actually have a benchmark to verify this statement and to measure the actual impact? I'm not saying there wouldn't be performance degradation, but I think the feature is important in scripting context, so we have to try and do something about it. Generally people don't use scripting when performance matters, they use it for convenience at least in our case (Code::Blocks). Also it is not the first time someone is requesting it. I've seen topics about it on the forum.

Here is a non-optimal version of patch for a variation of this problem:

diff --git a/src/sq3/squirrel/sqbaselib.cpp b/src/sq3/squirrel/sqbaselib.cpp
index e1d5c2b..7859b1e 100644
--- a/src/sq3/squirrel/sqbaselib.cpp
+++ b/src/sq3/squirrel/sqbaselib.cpp
@@ -647,7 +647,7 @@ static SQInteger array_find(HSQUIRRELVM v)
     for(SQInteger n = 0; n < size; n++) {
         bool res = false;
         a->Get(n,temp);
-        if(SQVM::IsEqual(temp,val,res) && res) {
+        if(v->IsEqual(temp,val,res) && res) {
             v->Push(n);
             return 1;
         }
diff --git a/src/sq3/squirrel/sqclass.cpp b/src/sq3/squirrel/sqclass.cpp
index 9845b83..f6b6512 100644
--- a/src/sq3/squirrel/sqclass.cpp
+++ b/src/sq3/squirrel/sqclass.cpp
@@ -76,7 +76,7 @@ bool SQClass::NewSlot(SQSharedState *ss,const SQObjectPtr &key,const SQObjectPtr
             }
             if(type(temp) == OT_NULL) {
                 bool isconstructor;
-                SQVM::IsEqual(ss->_constructoridx, key, isconstructor);
+                _thread(ss->_root_vm)->IsEqual(ss->_constructoridx, key, isconstructor);
                 if(isconstructor) {
                     _constructoridx = (SQInteger)_methods.size();
                 }
diff --git a/src/sq3/squirrel/sqvm.cpp b/src/sq3/squirrel/sqvm.cpp
index 2c9cdc8..f2f6f54 100644
--- a/src/sq3/squirrel/sqvm.cpp
+++ b/src/sq3/squirrel/sqvm.cpp
@@ -639,6 +639,22 @@ bool SQVM::IsEqual(const SQObjectPtr &o1,const SQObjectPtr &o2,bool &res)
 {
     if(type(o1) == type(o2)) {
         res = (_rawval(o1) == _rawval(o2));
+        if (!res && type(o1) == OT_INSTANCE) {
+            if(_delegable(o1)->_delegate) {
+                SQObjectPtr closure;
+                if(_delegable(o1)->GetMetaMethod(nullptr, MT_CMP, closure)) {
+                    Push(o1);Push(o2);
+                    SQObjectPtr cmpResult;
+                    if(CallMetaMethod(closure,MT_CMP,2,cmpResult)) {
+                        if(type(cmpResult) != OT_INTEGER) {
+                            Raise_Error(_SC("_cmp must return an integer"));
+                            return false;
+                        }
+                        res = _integer(cmpResult) == 0;
+                    }
+                }
+            }
+        }
     }
     else {
         if(sq_isnumeric(o1) && sq_isnumeric(o2)) {
@@ -646,6 +662,38 @@ bool SQVM::IsEqual(const SQObjectPtr &o1,const SQObjectPtr &o2,bool &res)
         }
         else {
             res = false;
+            if (type(o1) == OT_INSTANCE) {
+                if(_delegable(o1)->_delegate) {
+                    SQObjectPtr closure;
+                    if(_delegable(o1)->GetMetaMethod(nullptr, MT_CMP, closure)) {
+                        Push(o1);Push(o2);
+                        SQObjectPtr cmpResult;
+                        if(CallMetaMethod(closure,MT_CMP,2,cmpResult)) {
+                            if(type(cmpResult) != OT_INTEGER) {
+                                Raise_Error(_SC("_cmp must return an integer"));
+                                return false;
+                            }
+                            res = _integer(cmpResult) == 0;
+                        }
+                    }
+                }
+            }
+            else if (type(o2) == OT_INSTANCE) {
+                if(_delegable(o2)->_delegate) {
+                    SQObjectPtr closure;
+                    if(_delegable(o2)->GetMetaMethod(nullptr, MT_CMP, closure)) {
+                        Push(o1);Push(o2);
+                        SQObjectPtr cmpResult;
+                        if(CallMetaMethod(closure,MT_CMP,2,cmpResult)) {
+                            if(type(cmpResult) != OT_INTEGER) {
+                                Raise_Error(_SC("_cmp must return an integer"));
+                                return false;
+                            }
+                            res = _integer(cmpResult) == 0;
+                        }
+                    }
+                }
+            }
         }
     }
     return true;
diff --git a/src/sq3/squirrel/sqvm.h b/src/sq3/squirrel/sqvm.h
index e3bb758..1b4e72d 100644
--- a/src/sq3/squirrel/sqvm.h
+++ b/src/sq3/squirrel/sqvm.h
@@ -80,7 +80,7 @@ public:
     bool Clone(const SQObjectPtr &self, SQObjectPtr &target);
     bool ObjCmp(const SQObjectPtr &o1, const SQObjectPtr &o2,SQInteger &res);
     bool StringCat(const SQObjectPtr &str, const SQObjectPtr &obj, SQObjectPtr &dest);
-    static bool IsEqual(const SQObjectPtr &o1,const SQObjectPtr &o2,bool &res);
+    /*static*/ bool IsEqual(const SQObjectPtr &o1,const SQObjectPtr &o2,bool &res);
     bool ToString(const SQObjectPtr &o,SQObjectPtr &res);
     SQString *PrintObjVal(const SQObjectPtr &o);

This one doesn't always call the _cmp for instances. It first checks if the instance pointers are the same. In this case I think it is fair to assume that the instances are equal. So here is prediction on the perf impact:

1. We're now passing a this pointer to IsEqual. It might matter if IsEqual is not inlined, which happens often.
2. We do an additional check for res==false on every same-type compare
3. If the check in 2 fails we do another check if the type of o1 is instance
4. If it is instance we get really slow
5. For the different-type case we now do two additional type checks.
6. If we have an instance we get really slow as in 4

Can you tell me which of the slowdowns bothers you the most? Can we measure it? I think 4 and 6 might get optimized by either storing a flag on the instance object if there is a _cmp metamethod or not (we don't need deep searches for meta methods I suppose).

It seems the biggest impact would be when comparing instances, but this is not something people do very often in perf critical parts of the code, I suppose. Also it could be optimized to not do the metamethod searches by setting some flag on the instance.

And as last resort the whole thing could be wrapped in a define and then only people which want to use it could pay the performance hit.

p.s. 5 and 6 would be good to have, but I'm fine if this doesn't actually work. For us it would be useful/used when doing comparisons between our internal string object and squirrel strings.

[obfuscated]

@albertodemichelis Ping?

[obfuscated]

@albertodemichelis Ping?