[prev in list] [next in list] [prev in thread] [next in thread] 

List:       enlightenment-devel
Subject:    Re: [E-devel] [EGIT] [core/efl] master 02/04: eio: migrate efl.io.manager.ls to use Eina_Future.
From:       Cedric Bail <cedric () ddlm ! me>
Date:       2017-09-29 17:50:13
Message-ID: DqWV9ubagxS9hYRvq6d-cuIW4DI_HjnbyNVermTOxZI91UjSi5aNd5ycc5leleWeyEXRBtxR5Ncm6XIyvDmcpeIS-jN3Tvug84IV5eM91AY= () ddlm ! me
[Download RAW message or body]

Sent with [ProtonMail](https://protonmail.com) Secure Email.

> -------- Original Message --------
> Subject: Re: [E-devel] [EGIT] [core/efl] master 02/04: eio: migrate \
> efl.io.manager.ls to use Eina_Future. Local Time: September 29, 2017 5:56 AM
> UTC Time: September 29, 2017 12:56 PM
> From: barbieri@gmail.com
> To: Enlightenment developer list <enlightenment-devel@lists.sourceforge.net>
> 
> I found this commit too problematic, you can read my review bottom-up
> to save some time... if you kept the same future signature it would be
> much simpler.
> 
> On Thu, Sep 28, 2017 at 10:31 PM, Cedric BAIL <cedric.bail@free.fr> wrote:
> > static void
> > +_future_file_done_cb(void *data, Eio_File *handler)
> > +{
> > + Eina_Promise *p = data;
> > +
> > + eina_promise_resolve(p, eina_value_uint64_init(handler->length));
> > +}
> > +
> > +static void
> > +_future_file_error_cb(void *data,
> > + Eio_File *handler EINA_UNUSED,
> > + int error)
> > +{
> > + Eina_Promise *p = data;
> > +
> > + eina_promise_reject(p, error);
> > +}
> > +
> > +static void
> > _no_future(void *data, const Efl_Event *ev EINA_UNUSED)
> > {
> > Eio_File *h = data;
> > @@ -154,6 +172,31 @@ _cleanup_info_progress(void *data)
> > }
> > 
> > static void
> > +_future_string_cb(void *data EINA_UNUSED, Eio_File *handler, Eina_Array *gather)
> > +{
> > + EflIoPath paths = ecore_thread_local_data_find(handler->thread, ".paths");
> > + void *paths_data = ecore_thread_local_data_find(handler->thread, \
> > ".paths_data"); + Eina_Accessor *access;
> > + unsigned int count;
> > + Eina_Stringshare *s;
> > +
> > + if (!paths)
> > + {
> > + eina_array_free(gather);
> > + return ;
> > + }
> > +
> > + access = eina_array_accessor_new(gather);
> > + paths(paths_data, access);
> > +
> > + // Cleanup strings, accessor and array
> > + EINA_ACCESSOR_FOREACH(access, count, s)
> > + eina_stringshare_del(s);
> > + eina_accessor_free(access);
> > + eina_array_free(gather);
> 
> this is weird... why do you create an acceessor to clean gather here,
> but above you clean gather without deleting stringshares? It"s not
> clear to me why do we need 2 code blocks.

The accessor already exist and is given to the callback. I just recycled it as I \
prefer the EINA_ACCESSOR_FOREACH macro to the Eina_Array one.

> > +
> > +static void
> > _file_string_cb(void *data, Eio_File *handler, Eina_Array *gather)
> > {
> > Efl_Promise *p = data;
> > @@ -320,31 +363,36 @@ _efl_io_manager_stat_ls(Eo *obj,
> > return NULL;
> > }
> > 
> > -static Efl_Future *
> > +static Eina_Future *
> > _efl_io_manager_ls(Eo *obj,
> > Efl_Io_Manager_Data *pd EINA_UNUSED,
> > - const char *path)
> > + const char *path,
> > + void *paths_data, EflIoPath paths, Eina_Free_Cb paths_free_cb)
> > {
> > - Efl_Promise *p;
> > + Eina_Promise *p;
> > + Eina_Future *future;
> > Eio_File *h;
> > 
> > - Eo *loop = efl_loop_get(obj);
> > - p = efl_add(EFL_PROMISE_CLASS, loop);
> > + p = eina_promise_new(efl_loop_future_scheduler_get(obj),
> > + _efl_io_manager_future_cancel, NULL);
> > if (!p) return NULL;
> > + future = eina_future_new(p);
> 
> create future later, since if this fails (ie: ENOMEM), it will call
> _efl_io_manager_future_cancel(NULL) since promise_data_set(p, h) isn"t
> done.

Hum, I have not assumed ENOMEM on eina_future_new. Calling eio_file_cancel(NULL) is \
fine, but I should just break out of the function asap actually. Not really something \
we can recover from.

> > 
> > h = _eio_file_ls(path,
> > - _file_string_cb,
> > - _file_done_cb,
> > - _file_error_cb,
> > + _future_string_cb,
> > + _future_file_done_cb,
> > + _future_file_error_cb,
> > p);
> 
> it would be nice to convert this to use the Promise/Future interface,
> would simplify things a lot, basically just a creator helper and
> nothing else needs to be done.

This are internal function also used by the legacy code.

> > + ecore_thread_local_data_add(h->thread, ".paths", paths, NULL, EINA_TRUE);
> > + ecore_thread_local_data_add(h->thread, ".paths_data", paths_data, \
> > paths_free_cb, EINA_TRUE);
> 
> this is an ugly hack... come on! Just add a plain struct to
> _eio_file_ls() that would contain "p", "paths" and "paths_data".

If I start using a structure here, I multiply the number of function to implement and \
increase the code size for no good reason. This is also reuse existing infrastructure \
that take care of cleanup automatically for me reducing further the code. I do not \
see why do you think it is an "ugly hack". Alternative solution lead to more code and \
complexity.

> > static Eina_Future *
> > _efl_io_manager_xattr_set(Eo *obj,
> > Efl_Io_Manager_Data *pd EINA_UNUSED,
> > diff --git a/src/lib/eio/efl_io_manager.eo b/src/lib/eio/efl_io_manager.eo
> > index 8ad5443f01..b8ed9002e2 100644
> > --- a/src/lib/eio/efl_io_manager.eo
> > +++ b/src/lib/eio/efl_io_manager.eo
> > @@ -7,17 +7,32 @@ struct Eio.Data
> > size: uint; [[Size of private data]]
> > }
> > 
> > +function EflIoPath {
> 
> why not "_" between elements like in every other EFL function name:
> Efl_Io_Path... and maybe use a more descriptive name?

This is a the first use of a function callback in .eo done properly. Obviously there \
are problem that can be seen. Callbacks type are not defined in a namespace, so if I \
start using a "_", they will popup as such in C and C++ which looked weird to me. I \
am not to sure of the right solution here.

> > +function EflIoDirectInfo {
> 
> name here, use "_" and more descriptive name, this looks like a regular type...
> 
> > + paths: EflIoPath; [[Callback called for each packet of files found]]
> 
> packet? not too descriptive to me... is this called for each file? for
> all files? Or non-accumulative partial results that were found by the
> scanner thread? (I think the later, if so, try to explain a bit more
> and explain how the user is supposed to handle that, such as "if you
> need the full list, then walk the accessor and copy each item to your
> own array or list"). Also, is it called from a thread or main loop?

It is indeed the later, will try to phrase it better somehow. As for regarding \
thread, the current model in Eo make it impossible to have any kind of nice API with \
thread so there is absolutely nothing nowhere in EFL interface that expose anything \
done in a thread.

> > }
> > - return: future<uint64, const(array<string>)>; [[List of entries in path]]
> > + return: ptr(Eina.Future) @owned; [[Amount of files found during the listing of \
> > the directory]]
> 
> I rather keep the original signature, why not? Rather than exposing
> the callback (which as I said above is confusing), just return an
> EINA_VALUE_TYPE_ARRAY of EINA_VALUE_TYPE_STRING... as
> eina_array_count() is O(1), there is no need to expose the uint64
> (count).

You missunderstand the original signature. It is exactly the same. The array was send \
in the progress callback which is what the second parameter of future<> was about.

> With your new signature it just throw away the usefulness of Future,
> making it behave much more like an "event".
> 
> While I can still see usefulness for partial results, like to
> pre-populate an UI, I"d *NOT* do as you did, with a plain callback...
> rather return an Processor object which emits: "results" events (your
> callback) and "finished".
> 
> The future/promise could then be a wrapper over this object, waiting
> for "finished" and then resolving with the full array.

That is how I started, but it looks ugly to use. You build your request, connect your \
callbacks and finally start it. It really is way more line of code than it feels \
worth it.

As our main use case is to have dynamic UI that handle large directory just fine, I \
focused on it and the API reflect that. I want to preserve the callback that allow \
the partial update of the UI as things goes, but instead of having the future only \
hold cound, I could return the aggregated array.

Will that do for you ?

Cedric
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel


[prev in list] [next in list] [prev in thread] [next in thread]