Patchwork [for-1.5,1/2] qom: aggressively optimize qom casting

login
register
mail settings
Submitter Anthony Liguori
Date May 13, 2013, 8:31 p.m.
Message ID <1368477106-7579-1-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/243528/
State New
Headers show

Comments

Anthony Liguori - May 13, 2013, 8:31 p.m.
This patch adds a small typename cache to ObjectClass.  This allows
caching positive casts within each ObjectClass.  Benchmarking a
PPC workload provided by Aurelien, this patch eliminates every
single g_hash_table_lookup() happening during the benchmark (which
was about 2 million per-second).

With this patch applied, I get exactly the same performance (within
the margin of error) as with --disable-qom-cast-debug.

N.B. it's safe to cache typenames only from the _assert() macros
because they are always called with string literals.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 include/qom/object.h |  4 ++++
 qom/object.c         | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 2 deletions(-)
Paolo Bonzini - May 13, 2013, 8:54 p.m.
Il 13/05/2013 22:31, Anthony Liguori ha scritto:
> This patch adds a small typename cache to ObjectClass.  This allows
> caching positive casts within each ObjectClass.  Benchmarking a
> PPC workload provided by Aurelien, this patch eliminates every
> single g_hash_table_lookup() happening during the benchmark (which
> was about 2 million per-second).
> 
> With this patch applied, I get exactly the same performance (within
> the margin of error) as with --disable-qom-cast-debug.
> 
> N.B. it's safe to cache typenames only from the _assert() macros
> because they are always called with string literals.

Nice!  Perhaps (for 1.6?) we can cache other results than class, so that
interfaces are sped up as well.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Anthony Liguori - May 13, 2013, 9:04 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 13/05/2013 22:31, Anthony Liguori ha scritto:
>> This patch adds a small typename cache to ObjectClass.  This allows
>> caching positive casts within each ObjectClass.  Benchmarking a
>> PPC workload provided by Aurelien, this patch eliminates every
>> single g_hash_table_lookup() happening during the benchmark (which
>> was about 2 million per-second).
>> 
>> With this patch applied, I get exactly the same performance (within
>> the margin of error) as with --disable-qom-cast-debug.
>> 
>> N.B. it's safe to cache typenames only from the _assert() macros
>> because they are always called with string literals.
>
> Nice!  Perhaps (for 1.6?) we can cache other results than class, so that
> interfaces are sped up as well.

Yes, there's no reason not to store any computed data within a class.
There are very few of them within QEMU and it's very easy to get to.

> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks.  We should leave the --disable-qom-casts present for 1.5 but I'd
like to discuss reverting it for 1.6.  I'll send patches once 1.6 opens
up.

Regards,

Anthony Liguori
Paolo Bonzini - May 13, 2013, 9:13 p.m.
Il 13/05/2013 23:04, Anthony Liguori ha scritto:
> We should leave the --disable-qom-casts present for 1.5 but I'd like
> to discuss reverting it for 1.6.  I'll send patches once 1.6 opens up.

I disagree, the cost is small but still provably nonzero.

We should only remove it once interfaces start to be more pervasive (it
would be nice for example to replace the RTC's #ifdef TARGET_I386 with
an interface).

Paolo
Peter Maydell - May 13, 2013, 10:08 p.m.
On 13 May 2013 21:31, Anthony Liguori <aliguori@us.ibm.com> wrote:
> This patch adds a small typename cache to ObjectClass.  This allows
> caching positive casts within each ObjectClass.  Benchmarking a
> PPC workload provided by Aurelien, this patch eliminates every
> single g_hash_table_lookup() happening during the benchmark (which
> was about 2 million per-second).

That's a lot of hashtable lookups...

> --- a/include/qom/object.h
> +++ b/include/qom/object.h
> @@ -344,6 +344,8 @@ typedef void (ObjectUnparent)(Object *obj);
>   */
>  typedef void (ObjectFree)(void *obj);
>
> +#define OBJECT_CLASS_CAST_CACHE 4

Total nitpick, but shouldn't this be
OBJECT_CLASS_CAST_CACHE_SIZE ?

thanks
-- PMM
Anthony Liguori - May 14, 2013, 12:38 a.m.
Peter Maydell <peter.maydell@linaro.org> writes:

> On 13 May 2013 21:31, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> This patch adds a small typename cache to ObjectClass.  This allows
>> caching positive casts within each ObjectClass.  Benchmarking a
>> PPC workload provided by Aurelien, this patch eliminates every
>> single g_hash_table_lookup() happening during the benchmark (which
>> was about 2 million per-second).
>
> That's a lot of hashtable lookups...

I suspect it's due to a cast somewhere in a path checking for pending
interrupts or something like that.  That would get called quite often.

>
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -344,6 +344,8 @@ typedef void (ObjectUnparent)(Object *obj);
>>   */
>>  typedef void (ObjectFree)(void *obj);
>>
>> +#define OBJECT_CLASS_CAST_CACHE 4
>
> Total nitpick, but shouldn't this be
> OBJECT_CLASS_CAST_CACHE_SIZE ?

Ack, I need to respin anyway so I'll make the updates.

Thanks,

Anthony Liguori



>
> thanks
> -- PMM
Anthony Liguori - May 14, 2013, 4:11 p.m.
Applied.  Thanks.

Regards,

Anthony Liguori

Patch

diff --git a/include/qom/object.h b/include/qom/object.h
index 63e2a40..23fc048 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -344,6 +344,8 @@  typedef void (ObjectUnparent)(Object *obj);
  */
 typedef void (ObjectFree)(void *obj);
 
+#define OBJECT_CLASS_CAST_CACHE 4
+
 /**
  * ObjectClass:
  *
@@ -356,6 +358,8 @@  struct ObjectClass
     Type type;
     GSList *interfaces;
 
+    const char *cast_cache[OBJECT_CLASS_CAST_CACHE];
+
     ObjectUnparent *unparent;
 };
 
diff --git a/qom/object.c b/qom/object.c
index f5f416b..ec88231 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -439,7 +439,16 @@  Object *object_dynamic_cast_assert(Object *obj, const char *typename,
                                      typename, file, line, func);
 
 #ifdef CONFIG_QOM_CAST_DEBUG
-    Object *inst = object_dynamic_cast(obj, typename);
+    int i;
+    Object *inst;
+
+    for (i = 0; i < OBJECT_CLASS_CAST_CACHE; i++) {
+        if (obj->class->cast_cache[i] == typename) {
+            goto out;
+        }
+    }
+
+    inst = object_dynamic_cast(obj, typename);
 
     if (!inst && obj) {
         fprintf(stderr, "%s:%d:%s: Object %p is not an instance of type %s\n",
@@ -448,6 +457,15 @@  Object *object_dynamic_cast_assert(Object *obj, const char *typename,
     }
 
     assert(obj == inst);
+
+    if (obj == inst) {
+        for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
+            obj->class->cast_cache[i - 1] = obj->class->cast_cache[i];
+        }
+        obj->class->cast_cache[i - 1] = typename;
+    }
+
+out:
 #endif
     return obj;
 }
@@ -510,7 +528,16 @@  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
     trace_object_class_dynamic_cast_assert(class ? class->type->name : "(null)",
                                            typename, file, line, func);
 
-#ifndef CONFIG_QOM_CAST_DEBUG
+#ifdef CONFIG_QOM_CAST_DEBUG
+    int i;
+
+    for (i = 0; i < OBJECT_CLASS_CAST_CACHE; i++) {
+        if (class->cast_cache[i] == typename) {
+            ret = class;
+            goto out;
+        }
+    }
+#else
     if (!class->interfaces) {
         return class;
     }
@@ -523,6 +550,15 @@  ObjectClass *object_class_dynamic_cast_assert(ObjectClass *class,
         abort();
     }
 
+#ifdef CONFIG_QOM_CAST_DEBUG
+    if (ret == class) {
+        for (i = 1; i < OBJECT_CLASS_CAST_CACHE; i++) {
+            class->cast_cache[i - 1] = class->cast_cache[i];
+        }
+        class->cast_cache[i - 1] = typename;
+    }
+out:
+#endif
     return ret;
 }