RE: The iterator interface

[Philip Van Hoof <spam pvanhoof be>]

On Tue, 2006-08-08 at 17:27 +0300, Dirk-Jan Binnema nokia com wrote:

> 
> >There's one exception, and that is the build-in iterator fot 
> >TnyHeaderListModel. Adding unreffing and reffing there isn't 
> >needed as all item-access is locked already anyway.
> >
> >And adding the ref and unref would slowdown sorting. That one 
> >uses 'special' _nl functions anyway (so the normal _current 
> >implementation of that iterator should indeed return with an 
> >extra ref, the _nl one shouldn't).
> 
> Hmmmm... I would say that *all* the _current functions should behave
> the same. They're implementing the same interface after all.

Well, the _nl implementations don't really implement the interface. The
interface is also implemented by normal implementations. The _nl ones
are hacks to make it faster for the internal used iterator. The internal
used iterator also isn't really like a normal iterator. 

These hacks, however, do make sorting significantly faster. Each
operation you add to them, will happen a gazillion times. 

For example not using inline (so calling it as a function, something
g_object_ref and g_object_unref would introduce twice) slows sorting
down with three seconds (for 20,000 headers). With reffing and unreffing
I fear sorting would take 6 seconds longer. Just for the JMP introduced
by calling the _ref and _unref methods of gobject.

My suggestion :), let the _nl methods or TnyHeaderListIterator and
TnyHeaderListModel be what they are. And fix the functions that do
implement the TnyListIface and TnyIteratorIface interfaces.


> As the freeing (or not) of returned values is such an error-prone
> area anyway, having one of the interface implementations behave
> different from all the others is asking for problems.

Right but the interface implementation itself wouldn't behave different.

Only those internally used (static) functions that work like an
iterator, but aren't. Note that the _nl ones aren't public API at all.
They are never touched by the developer that will use the tinymail
framework. The GTK+ implementation of libtinymailui-gtk, uses it. 

They are an implementation detail.

> I think that (as mentioned before) simply adding 
> 	tny_iterator_iface_current_unowned () 
> (as an alternative to the boolean flag mentioned before)
> would be much better. That's only one extra function.

The functions don't implement tnyiteratoriface, they are internal and
specific to tnyheaderlistmodel.

> >ps. _nl stands for "no lock", because the locking happens at a 
> >higher level (in the model implementation). Some are by the 
> >way implemented in the model implementation and are marked as inline.
> 
> BTW, making these functions void, may help for something else
> as well - thread safety; in tny_header_list_iterator:
> 
> (lock)
> me->current = ....
> (unlock)
> 
> return me->current->data;
> 
> Now, me->current could become NULL just after the lock? 
> (Thread 2 acquires lock, sets me->current to NULL...)

What I often did is getting the pointer before the lock, and then
returning that pointer. Not returning me->current->data because that
would indeed possibly crash. Note that the _nl ones don't do this
locking. So they have been implemented in the fastest possible way.

The locking happens in the TnyHeaderListModel implementation. This is
also why they aren't implementations for the iterator itself. But rather
implementation details of TnyHeaderListModel.

> We would we need something like:
> 
> (lock)
> my_current = me->current = ....
> (unlock)
> 
> return my_current->data;
> 
> (And that will still go wrong when my_current->data is unreffed'd 
> in the meantime....)
> 
> So making 'm void safes some thread-safety issues.
> 
> tny_header_list_iterator_current does it correctly, as long as
> we assume nobody is unreffing the retval while it's being used.

ok

-- 
Philip Van Hoof, software developer at x-tend 
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
work: vanhoof at x-tend dot be 
http://www.pvanhoof.be - http://www.x-tend.be