On Tue, Aug 9, 2011 at 5:33 PM, Cedric BAIL <[email protected]> wrote:
> On Tue, Aug 9, 2011 at 2:22 PM, Gustavo Sverzut Barbieri
> <[email protected]> wrote:
> > Hi all, particularly cedric. I was to do the bindings for the new
> > ethumb_client_exists() API and got impressed by the number of
> > problems.
> >
> > - inconsistent api: void* is the first callback parameter, see
> > all EFL
>
> Agreed, but I followed previous callback format of Ethumb_Client, see
> Ethumb_Client_Generate_Cb. I have no problem about breaking this and
> follow the rest of the EFL API convention. In fact, I would really
> like that solution as I think Ethumb API is a mess right now.
typedef void (*Ethumb_Client_Generate_Cb)(void *data,
Ethumb_Client *client, int id, const char *file, const char *key,
const char *thumb_path, const char *thumb_key, Eina_Bool success);
as I said, void*data is the first...
What of Ethumb API is a mess now? Could you clarify? And are you
talking about Ethumb or Ethumb_Client?
> > - inconsistent api: missing ethumb_CLIENT prefix, like
> > Ethumb_Exists should be Ethumb_Client_Exists
>
> The data type, right ?
yes.
> > - strange api: if I have an Ethumb_Exists handle why do I need
> > all the other parameters? Because Ethumb_Exists is an internal
> > thing exposed to user! You should return the
> > Ethumb_Async_Exists_Cb structure (and remove Cb from it)
>
> Maybe I should reverse the logic, have all Cb pointing to the same
> Ethumb_Async_Exists instance, that would solve the cancel parameter
> things.
keep them split, listen to me :-)
> > - memory leak: in _ethumb_client_exists_end() you free the
> > async->callbacks but nothing frees 'cb' nodes.
>
> Outch and not only there, but also in cancel.
I see nothing wrong with cancel, actually it should call _end as well.
> > - strange behavior: you can create lots of async requests, but
> > if you cancel one, all of them is cancelled!
>
> Nop, only the callback is cancelled. It will wait to have no more
> callback before cancelling the job.
yeah, I failed to see it's just when the async->ref drops to zero...
damn I hate those refcount macros...
> > - race condition: thread may start and finish before you add the
> > structure to the hash:
> > async->thread = ecore_thread_run(...);
> > eina_hash_direct_add(_exists_request, async->dup, async);
>
> Yup, should check ecore_thread_run returned value.
I don't recall ecore_thread_run() behavior but I don't see how it
would help. It could have gathered the structure to return and right
before the function does the return the process can be preempted to
run the thread, that may finish it's stuff before being preempted. At
the end you return a dead thread. Like this:
thread-1: ecore_thread_run(), allocates return, start thread
thread-2: run and finishes
thread-1: return a finished thread
> > - strange behavior: provided cb is executed even when it was
> > cancelled
>
> Yes, it was done that way as it's the only way to be sure that the
> data are not used any more. But right now, if I reverse the loginc
> with callback and async stuff, it should not be needed anymore as
> callback will not be in the list anymore.
>
> > - bad API design: there is no free_cb for exists. If you cancel
> > it you still get the main cb but this is not clear. This is
> > bad for bindings.
>
> Agreed and unecessary now in fact, as the callback is removed from the
> list, there is no chance to be called later.
please add a free callback. It makes bindings (or anyone doing proper
code) a mess to delete references to it and cleanup.
> > In my opinion there should be no single-thread per ethumb_client
> > thing. It's a micro optimization and is useless since most users will
> > ask just one.
>
> I don't think so, fire elementary_test ethumb test and you will see
> the difference directly. Grouping command are usefull when used by
> gengrid as you can be very quickly asked to create one
> thumbnail,destroy and asked again, or some times asked twice and
> destroyed once. It was possible to see the difference in excessive
> also.
This is a problem with elementary's usage then. Although I really
really fail to see this happening a lot in common cases. You rarely
will have a thumb view with the same icon as we have in
elementary_test, that's just nonsense... we happen to have it as we
have few images to test, it's far from realistic.
Seriously, make it right first, if people come to provide a real use
case I can take a look at it myself.
All in all if that turns to be a common case I'd rather cache the
paths in elm_ethumb than to fix this the "optimization" we have now.
It is similar to my complains of the "exists" being async, god damn,
we're computing an MD5 of a path... this shouldn't be that much worse
than computing the eina_hash to go into a thread! Just check how other
parts of this process account and this part should be very minimal to
deserve such handling... look how the code grown, and which problems
went in for little benefit. If you think it is worth because it
stat()s the target file, then you can just wrap some eio call and
avoid all the mess around ethumb.
> Right now, the main problem of Ethumb are it's internal, to
> complex and really messy. Exposing Id is in my opinion a bad idea and
> the API is clearly a mess. This make it really difficult to fix the
> last issue I am facing with it, being to process one thumb per
> available CPU.
Again, what in ethumb? now looks like the server/daemon. What you mean
with "exposing ld", what "ld"? What is messy?
I really think that the ethumbd slave should be a single process as I
don't want it trashing even more a system. Let's see, if you're
getting lots of thumbnails requests:
1- likely you're doing lots of work on your application (highest
interactivity priority)
2- you'll be sending lots of dbus commands, that will consume work on its own
3- you'll wake the dbus-daemon to dispatch the commands
4- you'll wake the ethumbd to queue requests
5- you'll wake the slave to process the requests
that's a lot of processes already, I guess adding more to fight for
CPU will make things worse. Okay, you may end the generation faster,
but that was never the point. The point of having async/daemon is
really to keep the main application responsive while things are being
generated on the background.
Anyway, imposing a limitation is not something I want. There are
people with 16 cores out there and then it could be useful. In this
case I can guide you to get there. It should be simple:
1- on elementary use various clients (a poll of clients), maybe add
an API to check how many ethumbd slaves are enabled (if this is
configured somewhere)
2- on ethumbd creates various slaves and round-robin clients/requests to them
quite simple and direct. Possibly what you're trying to do is to mess
it all inside one single ethumb_client request and that is bad. Each
client is meant to be sequential fetching, although asynchronous. Just
look the amount of work to have this work... if you add simultaneous
to it, then it will get even worse, performance and usability (API)
wise.
> Anyway thanks for your review, Ethumb need some love and I almost
> stoped my work in the middle as it worked for me...
IMO just remove ethumb calls from threads, if you want to stat() from
a thread, then fine but shouldn't matter much. Fix the API as I said.
Leave multiple slaves out.
Take some time to understand how ethumb/ethumbd/ethumb_client works.
It is meant to be serial on each level and even so, due the async
nature, it is already quite complex.
Ideally it would be like:
handle = ethumb_generate(file, parameters, end_cb)
But C does not make it easy... 'parameters' there would be a mess by
itself. Either it would be a function with lots of parameters or a
struct you'd have to manage... If a structure you'd have to avoid
recreating/refilling it everytime, and then you'd face exactly the
same problems we do now and what you call it "messy".
The current API tries to be convenient and easy to use, yet efficient.
It will cache things, make similar requests easy, etc. Just
understand you better use it in a serial way.
--
Gustavo Sverzut Barbieri
http://profusion.mobi embedded systems
--------------------------------------
MSN: [email protected]
Skype: gsbarbieri
Mobile: +55 (19) 9225-2202
------------------------------------------------------------------------------
Get a FREE DOWNLOAD! and learn more about uberSVN rich system,
user administration capabilities and model configuration. Take
the hassle out of deploying and managing Subversion and the
tools developers use with it.
http://p.sf.net/sfu/wandisco-dev2dev
_______________________________________________
enlightenment-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
