After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 113116 - Consider caching of type check information
Consider caching of type check information
Status: RESOLVED INCOMPLETE
Product: glib
Classification: Platform
Component: gobject
2.2.x
Other Linux
: Normal normal
: ---
Assigned To: Tim Janik
gtkdev
Depends on:
Blocks:
 
 
Reported: 2003-05-16 13:36 UTC by Soren Sandmann Pedersen
Modified: 2011-02-18 16:13 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Soren Sandmann Pedersen 2003-05-16 13:36:11 UTC
This bug was originally bug 105500. 

Some profiles show significant time spent on type checking. The patch I am
attaching caches typecheck information in TypeNodes (ie., two words
per class). My limited tests indicate a hit rate of 85-95%, but I
haven't measured if it is actually an improvement, and if so how big
an improvement.

In bug 105500, Owen said:

I'm a little hesitant in general in putting caching in on
something that is in many cases constant time. (Constant
time for inheritance, log(n_interfaces) for interfaces.)
Since it's just going to slow things down on the miss
route and increase code size a bit.

Still, if benchmarking indicates that it makes a noticeable
improvement, it probably makes sense.
Comment 1 Kristian Rietveld 2003-06-06 23:53:03 UTC
In a testcase involving GtkTreeModel and GtkTreeModelFilter (though
with a small dataset, 3000 items) and some filtering, the cumulative
cost of g_type_instance_is_a went from ~7.14% before applying the
patch to ~1.85% after applying the patch.

According to Owen and me a not insignificant change.
Comment 2 Soren Sandmann Pedersen 2003-06-07 09:27:22 UTC
Do you have numbers on the effect in wall-clock time?
Comment 3 Tim Janik 2004-02-19 18:21:32 UTC
this patch misses the original attachment, aparently this is meant:

http://bugs.gnome.org/showattachment.cgi?attach_id=14528

but like owen, i'm hesitant to apply something like this. for ordinary
object types, an is_a check amounts to 2 array lookups for the type
nodes and one extra array lookup in the supers[] array. so the
expected benefit is pretty small. as a result, i need to see hard
actual numbers (like 1e+6 calls/sec without and 3e+6 calls/sec with
patch applied) for a likely real-world scenario to be convinced to
apply this.

on the technical side, the patch could certainly be rewritten to be a
little more compact in what it does, but more importantly, it caches
the last two instance_is_a check-types (which will be equal for a
large number of cases) as opposed to the last two *different*
check-types, which is likely to produce more hits.
but it's a hard call to make here anyway, whether we want 1, 2, 3 or 7
(different) cache comparisons. normally you'd pick a number that in
worst-case scenarios still far outweights the mis performance case -
but that's just 3 array accesses here...
Comment 4 Soren Sandmann Pedersen 2004-02-28 15:47:14 UTC
> it caches the last two instance_is_a check-types (which will be equal
> for a large number of cases) as opposed to the last two *different*
> check-types, which is likely to produce more hits.

That's not correct. If the type you are checking against is already in
the cache, you'll get a hit and no cache updating will take place.
Comment 5 Tim Janik 2005-07-27 02:48:24 UTC
you're right soeren, thanks for the correction.
i'm still waiting on the numbers though. 
considering the issues we encountered with the atomic pointer access
implementations (memory barriers etc.), i don't think relying on GType
assignments being atomic really is something we want to do.

instead, using either g_atomic_pointer_get()/swap() (unfortunately there's no
atomic_pointer_set() that just takes care of the memory barriers) or __thread
cache variables may still gain us improvements.

both aproaches aren't without catches though, atomic pointer accesses take a
bus/processor lock hit which is likely to even degrade performance, an using
__thread variable isn't highly portable, as it introduces minimum requirements
on the versions of gcc, glibc and the runtime linux kernel being used.

in any case, i'd need a reasonably representative standalone testcase that
produces numbers in favour of either caching variant and am inclined to close
this bug report if that isn't supplied.
Comment 6 Federico Mena Quintero 2005-11-08 19:41:30 UTC
Can we get some numbers on operations that are likely to do many type checks? 
For example, adding 10K rows to a tree view.
Comment 7 Tim Janik 2006-05-31 00:30:38 UTC
note that by now we have the required set of atomic operations in place.
to proceed with this report, we need:
- the patch to be resubmitted based on atomic accessors (comment #5)
- a testcase that exhibits the actual improvement, e.g. like sugegsted by Federico (comment #6)
Comment 8 Tim Janik 2006-08-23 08:51:47 UTC
i'm goign to close this in a bit if there is no further progress here.
setting to NEEDINFO now.
Comment 9 André Klapper 2008-07-19 11:13:51 UTC
Closing this bug report as no further information has been provided. Please feel free to reopen this bug if you can provide the information asked for.
Thanks!