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

[Mike Kestner]

Hi Ben,

Thanks for the patch.  

The following comments are intended not only as direct feedback on the
patch, but also an attempt to develop some standards for
performance-related patch submissions in the future.

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.

Specific comments below.

On Sun, 2004-02-15 at 00:05, Ben Maurer wrote:

> These two patches reduce the cost by quite a bit, over 50%. Also, the
> memory allocation is way down.

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.

> 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. 

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. 

> 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?  

> 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.  The memory gain seems to imply leaks
to me, since GLib.Values are transient.  Did you attempt to identify
where the leaks are?

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.

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%."  Finally, the above
measurements should be made on atomic changes, not an aggregation of
loosely related changes.

Thanks,
-- 
Mike Kestner <mkestner@ximian.com>