[Gtk-sharp-list] PATCH: speed up treeview and managed values

[Ben Maurer]

On Sun, 2004-02-15 at 11:00, Mike Kestner wrote:
> I take code complexity seriously.  Before I'll approve an increase in
> complexity over working existing code on performance reasons, I will
> need to see more definitive information on the performance gained.
Yep, I totally agree :-).

> Please don't do two things in one patch.  Especially in a performance
> improvement patch.  It just makes it harder to review the patch.  Also,
> as jluke mentioned, please attach patches instead of providing links to
> them.
Will attach two seperate patches in a seperate reply, with extra perf
data.

> > The first one implements ManagedValue using GCHandles. The bulk of this
> > code is now implemented in C to avoid some overhead (before, we were
> > accessing a hashtable by an intptr which caused boxing). Also, this
> > avoids the cost of invoking a C# delegate from C.
> 
> A couple of things on this part of the patch.  One, I don't think I
> really want to add a lot of glue code and complexity to the ManagedValue
> implementation, since it will be deprecated as soon as the NodeStore is
> completed. 
Actually, that code has alot of value, even outside the context of the
TreeStore. For example, lets say you have a callback like

DooFoo (..., gpointer user_data);

In C#, the only way to add some data here would be to use ManagedValue.

Also, it provides a great way to pass around C# objects in the managed
code.

I also hope that this can be the basis for any similar code in the
NodeStore, as I am sure something similar needs to be done.

> Second, this part of the patch is not written in a style consistent with
> the type declaration conventions in Gtk+.  For example, methods that
> return a GType are named my_type_name_get_type.  _init functions are
> used to initialize instances of a type. 
Ah, thank you. I will fix this.

> > The second patch avoids the allocation of Glib.Value's by implementing
> > the code in C where the values are stored on the stack.
> 
> This is just a brute force attack.  I would prefer the brute force
> behind GValues be buried inside GLib.Value instead of duplicated in
> every API that uses GValues.  Did you consider caching and block
> allocation of GLib.Value wrappers if allocation is the issue?  
Well, the real issue is that GValues should be allocated in the stack,
not inside the GC'd heap.

In my C code I have this:
	GValue value = { 0, };
	const <blah> ret;
	
	gtk_tree_model_get_value (store, iter, column, &value);
	ret = <get blah>;
	g_value_unset (&value);
	return ret;

Note that there are no calls to a malloc function. However, in C# we end
up allocating objects with pointers to GValues. It is possible to have
the same syntax in C# for example, we could do:

struct Glib.Value {
}

static extern void gtk_tree_model_get_value (..., out GLib.Value);

bool GetBool ()
{
    GLib.Value v;
    gtk_tree_model_get_value (..., out v);
    bool b = (bool) v;
    v.Unset ();
    return b;
}

However, this would require major changes. For example, note that I had
to explicitly call `Unset'. This is because you cant have a struct with
a finalizer (because finalization is a GC related thing, and structs
bypass the GC by design). We could implement IDisposeable, so the
pattern would look like:

bool GetBool ()
{
    using (GLib.Value v) {
       gtk_tree_model_get_value (..., out v);
       return (bool) b;
    }
}

However, the problem is that you still end up forcing the use of this
construct. Since GLib.Value should really only be used in the generated
code and in .custom files, I think it is still an option. I would be
happy to look into this.

> > Overall, this made a noticable difference when loading a large project
> > (in this case, the MCS compiler) in MonoDevelop. The time improved and
> > memory was down by a few MB (according to top).
> 
> Noticeable differences aren't the standard with performance issues. 
> Measurable differences are.  Time improved, but by how much?  "It seems
> faster" isn't a performance gain.
See my comments on the last paragraph.

> The memory gain seems to imply leaks
> to me, since GLib.Values are transient.  Did you attempt to identify
> where the leaks are?
Actually, this was not the case. I looked in the profiler after a bit of
execution time, the number of calls to GLib.Value ctors matched up with
the calls to the dtors, which implies that we are freeing things
correctly. Also, everything was eventually getting disposed in unmanaged
(seen because the dispose queue was being emptied).

The problem is that when we dispose a GLib.Value, we actually put it in
a queue that waits for idle time to dispose the values. When I was doing
sorting, there was no idle time. So, the values just sat there until it
was done, increasing memory usage. Also, the GC was not kicking in very
often (it was only doing it after a MB or two of allocation), so there
were alot of dead GLib.Values sitting around waiting for the next
collection.

> Which part of the patch improved time?  Which part of the patch improved
> memory?  By combining two different "fixes" in one patch like this, it
> is hard to understand if/why both parts are necessary.
Actually, both of these patches fix memory. The speed benefit comes in
two places. First, we reduce the amount of time spent allocating memory.
Second, the time spent freeing memory is also reduced.

The ManagedValue patch does one other thing to increase perfromance
which is avoiding calls on an unmanaged delegate.


> So, in summary, on performance improvement patches like this that add
> code complexity in order to improve performance, I will first need to
> see clear instrumented evidence of the problem.  I'm talking numeric
> profiled evidence, not "tree view seems slow."  Also, I need to see
> measurements of the improvement, and by this I don't mean "50%
> improvement".  I mean "the profile prior to the patch showed 10% of
> execution time in GetValue, now it's 5%."
Actually, when I said `50%' what i meant was `the profile before showed
10%, now it shows 5%'. I should have been more specific here.

Also, remember that this number is not the entire story. There is some
time spent in the GC directly due to the number of allocations, but it
is not shown because it does not always land somewhere where I can see
it in my profile.

>   Finally, the above
> measurements should be made on atomic changes, not an aggregation of
> loosely related changes.
Sorry, will get this data for you. In fact I am thinking of abandoning
the treemodel one and instead looking at the GLib.Value --> struct
change. Gimme a few minutes.

-- Ben