Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Thu, 4 Aug 2011 09:18:30 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: On Fri, 29 Jul 2011, Vincent Torri wrote: On Fri, 29 Jul 2011 06:57:33 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. no. It's not normal at all. of course it is! i've been doing this for YEARS! adding new features that u need to either compile-time enable or runtime enable to use/see. is not ecore-xcb exactly that? what about all of the loaders fo eavs, or engines... or... i can go on. except for ecore_xcb (my code, before devilhorns' changes), the code was always functional and of good, or high quality. According to cedric, the code here will not work at all on some important cases. and I repeat what i told you on IRC : these changes are in the most important part of the EFL : the main loop. Also, doing such critical change, even if it is optional, **just before** a release (if I'm not mistaken the release is for september) is just masochism (as people will use it if it is in a release, even if it is optional) so again, I'm against that commit. Add it later if you want (you're the maintianer, so you do what you want), but now, for such critical change, it's not good at all. i'm still waiting for your answer... i already said my piece. its not that complex (as a commit) and its turned off and it WORKS. NOW. it's showing any problems. there's a chunk of xcb work going on too - you want to remove that too? we haven't called a freeze yet. so you're jumping the gun. -- - Codito, ergo sum - I code, therefore I am -- The Rasterman (Carsten Haitzler)ras...@rasterman.com -- BlackBerryreg; DevCon Americas, Oct. 18-20, San Francisco, CA The must-attend event for mobile developers. Connect with experts. Get tools for creating Super Apps. See the latest technologies. Sessions, hands-on labs, demos much more. Register early save! http://p.sf.net/sfu/rim-blackberry-1 ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Thu, 4 Aug 2011 11:49:59 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: so, as discomfitor said, last year you removed some piece of code saying that it was too untested and the release being too close. There is the same case here, and you don't remove it... the other code was enabled. this one is not. -- - Codito, ergo sum - I code, therefore I am -- The Rasterman (Carsten Haitzler)ras...@rasterman.com -- BlackBerryreg; DevCon Americas, Oct. 18-20, San Francisco, CA The must-attend event for mobile developers. Connect with experts. Get tools for creating Super Apps. See the latest technologies. Sessions, hands-on labs, demos much more. Register early save! http://p.sf.net/sfu/rim-blackberry-1 ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, Jul 29, 2011 at 3:44 AM, Mike McCormack
mj.mccorm...@samsung.com wrote:
On 07/28/2011 10:35 PM, Cedric BAIL wrote:
As I don't want the grumpy guy that don't help. You now have
ecore_main_loop_thread_safe_call that should be the base for your
patch. And I am still not convinced that we need more than that.
People should just use that function when they are doing thread stuff
and we should advertise it in our thread spanking message.
OK, thanks for spending the time to write this.
If you know you're writing code that's in a thread your function is
great. Call it, and then schedule some work in the main loop, it's
good.
But for a library writer:
1) assume thread safety (1 line):
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
2) Using ecore_main_loop_thread_safe_call (5 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
/* will always be marshalled through pipe, even when not necessary */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
3) Using ecore_main_loop_thread_safe_call, trying to be a bit more
efficient (8 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
if (!ecore_is_main_loop_thread()) /* assuming this function exists */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
else
_event_main_loop_callback();
Case 2 3 are more verbose, and you need to add such code for *every*
call to ecore (in a library).
So you end up pushing responsibilities to the library writers and
applications that are better handled in Ecore itself.
IOW, it makes writing ecore code with threads a pain in the butt,
and reduces the potential audience of EFL.
After re-reading that thread, I think you misunderstood one part of
this function. If I added it, it's for you to use it to implement your
thread safe idea, as it will provide you the easiest way and a gloal
infra to solve the awakening of the main loop. And it also prove that
we don't need much lock in the main loop at all to make it thread
safe.
--
Cedric BAIL
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Mon, Aug 1, 2011 at 4:33 AM, Mike McCormack mj.mccorm...@samsung.com wrote:
On 07/29/2011 05:56 PM, Cedric BAIL wrote:
Then take that pseudo example :
Eina_Bool _timer_in_main_loop(void *data)
{
;
lock_data_mutex();
if (!data_was_deleted)
{
free(data);
}
data_was_deleted = Eina_True;
unlock_data_mutex();
return 0;
}
void bad_idea()
{
tuttut = ecore_timer_add(unlucky, _timer_in_main_loop, something);
...;
if (bad_happen)
{
lock_data_mutex();
if (!data_was_deleted)
{
ecore_timer_del(tuttut);
free(something);
}
data_was_deleted = Eina_True;
unlock_data_mutex();
}
}
This code will work just fine in the main loop. Now in a thread it's
full of race condition and double free. Tell me how you can make it
work ?
Add a mutex. See above.
Nobody said threaded programming is easy. I'm just adding one tool.
Maybe you don't want to use this tool, that's OK. Maybe you want to
provide a different way to do it, that's OK too. Maybe you want to
make a law against using threads with ecore in EFL code, that's OK too.
Saying the tool itself is bad is a non-started, IMO.
The fact is, people already write this kind of racy code. They think
it works, and when it crashes in ecore, EFL gets the blame.
So you can stand stubbornly refuse to fix it, or move the problem to
their code, so it will crash outside of ecore, and people will not
blame EFL.
That's were we really disagree I think. I really think that you can't
protect the EFL yet against this kind of race condition and be sure
that the issue will show up in the backtrace of their app and not in
EFL. I really know and understand why you want to do that. I really
think that telling people that don't understand thread to not use them
or direct them to use the right function to make the call in the main
loop would be the only solution to reduce the burden put on you. But
if you really think that by making it thread safe, you will have less
false positive bug report, then fine. Use the function I added to
ecore.c to do your patch, and you should put some effort on my
prototype of Eina_Object as I think it will help to solve some race
condition to.
Additionally, people who grok threads well (admittedly not many)
can write code that works with ecore main loop.
They already can.
Support choice, and suggest the best way.
As I say from the beginning, I am really not for that stuff in EFL,
but if you do it fine, I will not oppose it.
--
Cedric BAIL
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On 07/29/2011 05:56 PM, Cedric BAIL wrote:
Then take that pseudo example :
Eina_Bool _timer_in_main_loop(void *data)
{
;
lock_data_mutex();
if (!data_was_deleted)
{
free(data);
}
data_was_deleted = Eina_True;
unlock_data_mutex();
return 0;
}
void bad_idea()
{
tuttut = ecore_timer_add(unlucky, _timer_in_main_loop, something);
...;
if (bad_happen)
{
lock_data_mutex();
if (!data_was_deleted)
{
ecore_timer_del(tuttut);
free(something);
}
data_was_deleted = Eina_True;
unlock_data_mutex();
}
}
This code will work just fine in the main loop. Now in a thread it's
full of race condition and double free. Tell me how you can make it
work ?
Add a mutex. See above.
Nobody said threaded programming is easy. I'm just adding one tool.
Maybe you don't want to use this tool, that's OK. Maybe you want to
provide a different way to do it, that's OK too. Maybe you want to
make a law against using threads with ecore in EFL code, that's OK too.
Saying the tool itself is bad is a non-started, IMO.
The fact is, people already write this kind of racy code. They think
it works, and when it crashes in ecore, EFL gets the blame.
So you can stand stubbornly refuse to fix it, or move the problem to
their code, so it will crash outside of ecore, and people will not
blame EFL.
Additionally, people who grok threads well (admittedly not many)
can write code that works with ecore main loop.
Support choice, and suggest the best way.
As far as I know, Cocoa, GTK, Qt and Win32 are not thread safe, and
nobody complain about that.
I know Win32's main loop is thread safe.
thanks,
Mike
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, Jul 29, 2011 at 3:44 AM, Mike McCormack
mj.mccorm...@samsung.com wrote:
On 07/28/2011 10:35 PM, Cedric BAIL wrote:
As I don't want the grumpy guy that don't help. You now have
ecore_main_loop_thread_safe_call that should be the base for your
patch. And I am still not convinced that we need more than that.
People should just use that function when they are doing thread stuff
and we should advertise it in our thread spanking message.
OK, thanks for spending the time to write this.
If you know you're writing code that's in a thread your function is
great. Call it, and then schedule some work in the main loop, it's
good.
But for a library writer:
1) assume thread safety (1 line):
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
2) Using ecore_main_loop_thread_safe_call (5 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
/* will always be marshalled through pipe, even when not necessary */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
3) Using ecore_main_loop_thread_safe_call, trying to be a bit more
efficient (8 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
if (!ecore_is_main_loop_thread()) /* assuming this function exists */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
else
_event_main_loop_callback();
Case 2 3 are more verbose, and you need to add such code for *every*
call to ecore (in a library).
Case 3, could be easily merged and done directly in
ecore_main_loop_thread_safe_call. Patch welcome, if I don't get to it
before anyone.
So you end up pushing responsibilities to the library writers and
applications that are better handled in Ecore itself.
IOW, it makes writing ecore code with threads a pain in the butt,
and reduces the potential audience of EFL.
Then take that pseudo example :
Eina_Bool _timer_in_main_loop(void *data)
{
;
free(data);
return 0;
}
void bad_idea()
{
tuttut = ecore_timer_add(unlucky, _timer_in_main_loop, something);
...;
if (bad_happen)
{
ecore_timer_del(tuttut);
free(something);
}
}
This code will work just fine in the main loop. Now in a thread it's
full of race condition and double free. Tell me how you can make it
work ? By trying to say, we are thread safe, you are just hidding more
and more the issue without solving it. So at the end, app will be less
stable and more difficult to debug. That's my point. Your patch isn't
fixing what it is advertising and I think it can't do it by design. C
is very different from Java, and is very difficult to make it thread
safe.
As far as I know, Cocoa, GTK, Qt and Win32 are not thread safe, and
nobody complain about that. It doesn't prevent anyone from writing
thousand of greate Cocoa appication (Maybe because they have GCD and
block and advertise it alot).
PS: Don't focus yet on performance cost, I didn't raise that point
yet, first we need something that do what it says it does before
trying any optimisation and we are far from that.
--
Cedric BAIL
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Wed, 27 Jul 2011 20:17:14 +0900 Mike McCormack mj.mccorm...@samsung.com said: it's 9pm on a friday night... and i don't think i'm going to get stuck into this right now. for now.. no one revert or do anything... i'll get to this tomorrow after some good sleep. :) Hi All, This patch adds some level of thread safety to ecore. It does not add thread awareness (i.e. adding a timer from a thread will not wake a sleeping main loop). In my experience, developers either are ignorant of the problems of threads, or expect libraries to magically work with threads. I understand that some people think that thread safety is not necessary, but IMO, the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. thanks, Mike -- - Codito, ergo sum - I code, therefore I am -- The Rasterman (Carsten Haitzler)ras...@rasterman.com -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, Jul 29, 2011 at 5:56 AM, Cedric BAIL cedric.b...@free.fr wrote:
On Fri, Jul 29, 2011 at 3:44 AM, Mike McCormack
mj.mccorm...@samsung.com wrote:
On 07/28/2011 10:35 PM, Cedric BAIL wrote:
As I don't want the grumpy guy that don't help. You now have
ecore_main_loop_thread_safe_call that should be the base for your
patch. And I am still not convinced that we need more than that.
People should just use that function when they are doing thread stuff
and we should advertise it in our thread spanking message.
OK, thanks for spending the time to write this.
If you know you're writing code that's in a thread your function is
great. Call it, and then schedule some work in the main loop, it's
good.
But for a library writer:
1) assume thread safety (1 line):
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
2) Using ecore_main_loop_thread_safe_call (5 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
/* will always be marshalled through pipe, even when not necessary */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
3) Using ecore_main_loop_thread_safe_call, trying to be a bit more
efficient (8 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
if (!ecore_is_main_loop_thread()) /* assuming this function exists */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
else
_event_main_loop_callback();
Case 2 3 are more verbose, and you need to add such code for *every*
call to ecore (in a library).
Case 3, could be easily merged and done directly in
ecore_main_loop_thread_safe_call. Patch welcome, if I don't get to it
before anyone.
So you end up pushing responsibilities to the library writers and
applications that are better handled in Ecore itself.
IOW, it makes writing ecore code with threads a pain in the butt,
and reduces the potential audience of EFL.
Then take that pseudo example :
Eina_Bool _timer_in_main_loop(void *data)
{
;
free(data);
return 0;
}
void bad_idea()
{
tuttut = ecore_timer_add(unlucky, _timer_in_main_loop, something);
...;
if (bad_happen)
{
ecore_timer_del(tuttut);
free(something);
}
}
This code will work just fine in the main loop. Now in a thread it's
full of race condition and double free. Tell me how you can make it
work ? By trying to say, we are thread safe, you are just hidding more
and more the issue without solving it. So at the end, app will be less
stable and more difficult to debug. That's my point. Your patch isn't
fixing what it is advertising and I think it can't do it by design. C
is very different from Java, and is very difficult to make it thread
safe.
As far as I know, Cocoa, GTK, Qt and Win32 are not thread safe, and
nobody complain about that. It doesn't prevent anyone from writing
thousand of greate Cocoa appication (Maybe because they have GCD and
block and advertise it alot).
I don't know about the others, but at least GTK is kind of thread
safe. You can't ignore that you are using threads and just add
callbacks at will or check for them, but at least you have some ways
to do it:
http://developer.gnome.org/gtk-faq/stable/x481.html
--
Rafael Antognolli
ProFUSION embedded systems
http://profusion.mobi
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, 29 Jul 2011, Rafael Antognolli wrote:
On Fri, Jul 29, 2011 at 5:56 AM, Cedric BAIL cedric.b...@free.fr wrote:
On Fri, Jul 29, 2011 at 3:44 AM, Mike McCormack
mj.mccorm...@samsung.com wrote:
On 07/28/2011 10:35 PM, Cedric BAIL wrote:
As I don't want the grumpy guy that don't help. You now have
ecore_main_loop_thread_safe_call that should be the base for your
patch. And I am still not convinced that we need more than that.
People should just use that function when they are doing thread stuff
and we should advertise it in our thread spanking message.
OK, thanks for spending the time to write this.
If you know you're writing code that's in a thread your function is
great. Call it, and then schedule some work in the main loop, it's
good.
But for a library writer:
1) assume thread safety (1 line):
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
2) Using ecore_main_loop_thread_safe_call (5 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
/* will always be marshalled through pipe, even when not necessary */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
3) Using ecore_main_loop_thread_safe_call, trying to be a bit more
efficient (8 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
if (!ecore_is_main_loop_thread()) /* assuming this function exists */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
else
_event_main_loop_callback();
Case 2 3 are more verbose, and you need to add such code for *every*
call to ecore (in a library).
Case 3, could be easily merged and done directly in
ecore_main_loop_thread_safe_call. Patch welcome, if I don't get to it
before anyone.
So you end up pushing responsibilities to the library writers and
applications that are better handled in Ecore itself.
IOW, it makes writing ecore code with threads a pain in the butt,
and reduces the potential audience of EFL.
Then take that pseudo example :
Eina_Bool _timer_in_main_loop(void *data)
{
;
free(data);
return 0;
}
void bad_idea()
{
tuttut = ecore_timer_add(unlucky, _timer_in_main_loop, something);
...;
if (bad_happen)
{
ecore_timer_del(tuttut);
free(something);
}
}
This code will work just fine in the main loop. Now in a thread it's
full of race condition and double free. Tell me how you can make it
work ? By trying to say, we are thread safe, you are just hidding more
and more the issue without solving it. So at the end, app will be less
stable and more difficult to debug. That's my point. Your patch isn't
fixing what it is advertising and I think it can't do it by design. C
is very different from Java, and is very difficult to make it thread
safe.
As far as I know, Cocoa, GTK, Qt and Win32 are not thread safe, and
nobody complain about that. It doesn't prevent anyone from writing
thousand of greate Cocoa appication (Maybe because they have GCD and
block and advertise it alot).
I don't know about the others, but at least GTK is kind of thread
safe.
gtk is not thread safe. glib is.
Vincent
You can't ignore that you are using threads and just add
callbacks at will or check for them, but at least you have some ways
to do it:
http://developer.gnome.org/gtk-faq/stable/x481.html
--
Rafael Antognolli
ProFUSION embedded systems
http://profusion.mobi
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Wed, 27 Jul 2011 15:08:21 +0200 Cedric BAIL cedric.b...@free.fr said: OK... quick comments: 1. adding threadsafety isnt bad. it's no worse than the thread checks NO ONe complained about. it's also optionally enabled, so i see no downside there. 2. yes - it's a pain in the butt to do, BUT if someone is willing to do it... should we say no to someone picking up the work and doing it? 3. thread safety doesnt fix thread awareness. lets remember that. that's a different problem to fix and a separate topic. right now we hsave no official way to wake up the main loop when u do things like add a timer in a thread - if ecore is threadsafe. you can create a mechanism via ecore_pipe - but a simpler one would be needed eventually. until it's ready and well tested we can still claim ecore is not threadsafe. so.. now to specific comments. Hi, On Wed, Jul 27, 2011 at 1:17 PM, Mike McCormack mj.mccorm...@samsung.com wrote: This patch adds some level of thread safety to ecore. It does not add thread awareness (i.e. adding a timer from a thread will not wake a sleeping main loop). So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. yes - it's not a final fix, then again how many things do we have in svn that get put in in stages. :) this doesnt finish making ecore threadsafe. it hasn't had a lot of testing yet. :) In my experience, developers either are ignorant of the problems of threads, or expect libraries to magically work with threads. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? they understand ecore_thread even less unfortunately. they also DONT understand ecore is NOT threadsafe. :( i sure as hell wasn't into the idea of making ecore threadsafe. i dont want to do the work. most here don't, but if someone is willing to do the work.. why should be veto it? I understand that some people think that thread safety is not necessary, but IMO, the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not well that requires evas be threadsafe and edje and elementary ... etc. - they are still yet to be done. until we get that far, this is a moot point. when we DO get that far... we can add things like: evas_begin(evas); evas_end(evas); that act like a freeze push and pop and when you get back to 0, you wake up the mainloop - via the evas event fd :) we can create a thread local counter for each of these so this increments per thread. we can lock the canvas while itds begun, or create more imaginative schemes, but to even get that far, we MUST make all the libs threadsafe anyway. possible, you are adding stuff to make it work more often, but that's not thread safety and will lead to more complex issue to debug in the futur. I would prefer that we advocate the use of ecore thread and use eina thread debugging property to prevent efl call from outside of the main loop by either asserting, displaying a backtrace or just spanking (stuff that could turned on and off at compile time and help provide development build or production build). education though is much harder. much much much much much harder. we can push these, but this does NOT preclude threadsafety. I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). -- Cedric BAIL -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel -- - Codito, ergo sum - I code, therefore I am -- The Rasterman (Carsten Haitzler)ras...@rasterman.com -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Thu, 28 Jul 2011 14:31:53 +0200 Cedric BAIL cedric.b...@free.fr said: Before providing a benchmark, please revert your broken code and provide a working solution based on my discussion with gustavo (using Ecore_Pipe). Right know, and before doing benchmark, your solution doesn't work in all case and that's a no go for me. It's already painfull for us when we use thread that I don't want to dig why some user have weird behaviour due to this half working solution. why revert? we simply dont claim ecore is threadsafe yet. in fact its not even enabled by default. -- - Codito, ergo sum - I code, therefore I am -- The Rasterman (Carsten Haitzler)ras...@rasterman.com -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, 29 Jul 2011 06:57:33 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. no. It's not normal at all. of course it is! i've been doing this for YEARS! adding new features that u need to either compile-time enable or runtime enable to use/see. is not ecore-xcb exactly that? what about all of the loaders fo eavs, or engines... or... i can go on. -- - Codito, ergo sum - I code, therefore I am -- The Rasterman (Carsten Haitzler)ras...@rasterman.com -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Sat, 30 Jul 2011, Carsten Haitzler (The Rasterman) wrote: On Fri, 29 Jul 2011 06:57:33 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. no. It's not normal at all. of course it is! i've been doing this for YEARS! adding new features that u need to either compile-time enable or runtime enable to use/see. is not ecore-xcb exactly that? what about all of the loaders fo eavs, or engines... or... i can go on. except for ecore_xcb (my code, before devilhorns' changes), the code was always functional and of good, or high quality. According to cedric, the code here will not work at all on some important cases. Vincent -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, 29 Jul 2011, Vincent Torri wrote: On Sat, 30 Jul 2011, Carsten Haitzler (The Rasterman) wrote: On Fri, 29 Jul 2011 06:57:33 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. no. It's not normal at all. of course it is! i've been doing this for YEARS! adding new features that u need to either compile-time enable or runtime enable to use/see. is not ecore-xcb exactly that? what about all of the loaders fo eavs, or engines... or... i can go on. except for ecore_xcb (my code, before devilhorns' changes), the code was always functional and of good, or high quality. According to cedric, the code here will not work at all on some important cases. and I repeat what i told you on IRC : these changes are in the most important part of the EFL : the main loop. Also, doing such critical change, even if it is optional, **just before** a release (if I'm not mistaken the release is for september) is just masochism (as people will use it if it is in a release, even if it is optional) so again, I'm against that commit. Add it later if you want (you're the maintianer, so you do what you want), but now, for such critical change, it's not good at all. Vincent -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, 29 Jul 2011 17:20:24 +0200 (CEST) Vincent Torri vto...@univ-evry.fr wrote: On Fri, 29 Jul 2011, Vincent Torri wrote: On Sat, 30 Jul 2011, Carsten Haitzler (The Rasterman) wrote: On Fri, 29 Jul 2011 06:57:33 +0200 (CEST) Vincent Torri vto...@univ-evry.fr said: Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. no. It's not normal at all. of course it is! i've been doing this for YEARS! adding new features that u need to either compile-time enable or runtime enable to use/see. is not ecore-xcb exactly that? what about all of the loaders fo eavs, or engines... or... i can go on. except for ecore_xcb (my code, before devilhorns' changes), the code was always functional and of good, or high quality. According to cedric, the code here will not work at all on some important cases. and I repeat what i told you on IRC : these changes are in the most important part of the EFL : the main loop. Also, doing such critical change, even if it is optional, **just before** a release (if I'm not mistaken the release is for september) is just masochism (as people will use it if it is in a release, even if it is optional) so again, I'm against that commit. Add it later if you want (you're the maintianer, so you do what you want), but now, for such critical change, it's not good at all. Vincent I'm normally gung ho about new features and all, but it does seem like we're cutting it close by sticking this in only a month or two before a possible release. If I recall (and I do since I have my mailbox open and flames are spewing out), I tried to add threadsafety to a few eina data types before 1.0. Despite having it functional and being 2-3 months before our eventual release, I bowed to the great wisdom of vtorri and reverted it. A similar situation happened with a match from Mike M. which optimized some main loop functionality by converting lists to inlists. On top of that, even if it's disabled by default this brings a radical shift in EFL's core philosophy. If we're going to implement it then it should be with general agreement (which is definitely not the case at present) and proper evaluations/benchmarks/etc which should NOT be rushed or ignored due to a pending release. -- Mike Blumenkrantz Zentific: Coding in binary since '10. -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Thu, Jul 28, 2011 at 7:20 AM, Mike McCormack mj.mccorm...@samsung.com wrote: On 07/27/2011 10:08 PM, Cedric BAIL wrote: So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. It is a change it goal. The goal will be to make ecore calls thread safe, and any non-thread safe behavior is a bug. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? I disagree with people should never use things they don't understand. The whole point of a library or any other programming abstraction is to hide details and save people reimplementing these things over and over themselves. Some developers in Samsung use ecore_thread, and some don't. Honestly, ECORE_MAIN_LOOP_ASSERT has shown up quite a few problems with code. Though the majority of developers understand that EFL code should be single threaded, there are still many crashes due to threading issues. I think EFL will be seen as more stable if it is thread safe... thus these patches. Maybe stable, but the rendered scene will not make sense and be highly difficult to debug. So spank and point people to the right solution. If you try to avoid segv by making code that are buggy almost working in most case, you are trying to hide issue and they will bite you harder when they come back. I think we should display error message that people do understand well and that point them to where they should fix their code and how they should fix it. And I am sure we can provide such a warning. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not Let's stick to technical stuff rather than abstract ideas like sanity. I'm only talking about ecore here. Step by step... (Raster has big plans for the rendering engine involving threads.) As far as I understand raster plan for rendering, it doesn't involve the user api of evas at all. It will continue to be a seriealised list of function call from the same main thread, then something will trigger evas_render and compute what is needed for doing the rendering asynchronously, before waking up all the rendering thread and getting out of evas_render and back to the application logique. And if you want you can put all this call in another thread, but it will always require to come from the same thread or they will never be any synchronization of the object state before entering the rendering state. If the code is lacking in one area or the other, please point it out. (Actually, some big bits, like locks around the idlers are missing from the patch I sent.) See my answer to gustavo if you want something that work in all case. I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). Well three points: * nobody complained when I added checks to show that ecore calls are coming from the same thread, even though the performance impact would be comparable. (See ECORE_MAIN_LOOP_ASSERT) Because if needed, we can turn it off without breaking application or any behaviour. Something that was working with it on will work with it off. It something that we could use in a development build and remove from the production build without the need to worry about it at all. * given that SMP is widely available these days, I would think that enabling simple thread based programming could dramatically improve performance of some code. Agreed, that's why we have Ecore_Thread in Ecore (stuff that I added) * I will provide a way to switch off thread safety at build time That wouldn't help, because you will break application. There is no way to have a development build and a production build with that kind of feature, either you have only one application in your system and you know that it doesn't require Ecore to be thread safe and you can turn it off, or you just can't turn this feature off. Right now my thoughts are: * support single main loop only * add support for waking the main loop when timers, fds, etc. are added I thing that this will get confusing and people will introduce more bug, because the barrier with thread safety will be blured, some function call will work, some won't. At the end, I think you are increasing the complexity of the EFL without any benefit, you will have more and more complex code to debug (because every thing will not be thread safe). I really am more for a macro that display a spank with backtrace and do nothing in all efl function call (including evas, edje and elementary), that point to Ecore_Thread documentation. In fact halt of that code is already provided by eina, I can provide it quickly if that feat your need. This will put the burden of the fix on the
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On 07/28/2011 07:32 PM, Cedric BAIL wrote: Maybe stable, but the rendered scene will not make sense and be highly difficult to debug. So spank and point people to the right solution. This is ecore, not evas. Adding timers from a thread makes sense to me. I'm not proposing this as a solution for Evas. Perhaps adding a thread check is better there... I haven't looked at it closely. * nobody complained when I added checks to show that ecore calls are coming from the same thread, even though the performance impact would be comparable. (See ECORE_MAIN_LOOP_ASSERT) Because if needed, we can turn it off without breaking application or any behaviour. Something that was working with it on will work with it off. It something that we could use in a development build and remove from the production build without the need to worry about it at all. If you're worried about this, how about I add a function call that can set the _ecore_lock() and _ecore_unlock() functions. If they are NULL, the won't be called. Also, if you worry so much about performance, please let me know what benchmarking you want done to show the overhead. I thing that this will get confusing and people will introduce more bug, because the barrier with thread safety will be blured, some function call will work, some won't. At the end, I think you are increasing the complexity of the EFL without any benefit, you will have more and more complex code to debug (because every thing will not be thread safe). I really am more for a macro that display a spank with backtrace and do nothing in all efl function call (including evas, edje and elementary), that point to Ecore_Thread documentation. In fact halt of that code is already provided by eina, I can provide it quickly if that feat your need. This will put the burden of the fix on the developer and not you. They will know exactly where and how to fix it. I have already added code to print warnings. It helps, but the response is usually, but why isn't EFL thread safe? Adding thread safety means: * one whole class of API misuse and potential problems is gone * people who think they know what they're doing can use threads without worrying that ecore is a source of problems * we offer the same features to 3rd parties as we want to use inside EFL. It's a bit hypocritical to say Well, the EVAS code can use threads, because we're E-lite developers, but users of our API should never be able to use threads, and should get spanked for doing so. thanks, Mike -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Thu, 28 Jul 2011, Mike McCormack wrote: On 07/28/2011 07:32 PM, Cedric BAIL wrote: Maybe stable, but the rendered scene will not make sense and be highly difficult to debug. So spank and point people to the right solution. This is ecore, not evas. Adding timers from a thread makes sense to me. I'm not proposing this as a solution for Evas. Perhaps adding a thread check is better there... I haven't looked at it closely. * nobody complained when I added checks to show that ecore calls are coming from the same thread, even though the performance impact would be comparable. (See ECORE_MAIN_LOOP_ASSERT) Because if needed, we can turn it off without breaking application or any behaviour. Something that was working with it on will work with it off. It something that we could use in a development build and remove from the production build without the need to worry about it at all. If you're worried about this, how about I add a function call that can set the _ecore_lock() and _ecore_unlock() functions. If they are NULL, the won't be called. Also, if you worry so much about performance, please let me know what benchmarking you want done to show the overhead. I thing that this will get confusing and people will introduce more bug, because the barrier with thread safety will be blured, some function call will work, some won't. At the end, I think you are increasing the complexity of the EFL without any benefit, you will have more and more complex code to debug (because every thing will not be thread safe). I really am more for a macro that display a spank with backtrace and do nothing in all efl function call (including evas, edje and elementary), that point to Ecore_Thread documentation. In fact halt of that code is already provided by eina, I can provide it quickly if that feat your need. This will put the burden of the fix on the developer and not you. They will know exactly where and how to fix it. I have already added code to print warnings. It helps, but the response is usually, but why isn't EFL thread safe? tell them to read the preamble there : http://docs.enlightenment.org/auto/ecore/Ecore_Main_Loop_Page.html Vincent Adding thread safety means: * one whole class of API misuse and potential problems is gone * people who think they know what they're doing can use threads without worrying that ecore is a source of problems * we offer the same features to 3rd parties as we want to use inside EFL. It's a bit hypocritical to say Well, the EVAS code can use threads, because we're E-lite developers, but users of our API should never be able to use threads, and should get spanked for doing so. thanks, Mike -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Thu, 28 Jul 2011 12:45:14 +1000 David Seikel onef...@gmail.com wrote: On Wed, 27 Jul 2011 15:08:21 +0200 Cedric BAIL cedric.b...@free.fr wrote: On Wed, Jul 27, 2011 at 1:17 PM, Mike McCormack mj.mccorm...@samsung.com wrote: This patch adds some level of thread safety to ecore. It does not add thread awareness (i.e. adding a timer from a thread will not wake a sleeping main loop). So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. In my experience, developers either are ignorant of the problems of threads, or expect libraries to magically work with threads. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? I understand that some people think that thread safety is not necessary, but IMO, the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not possible, you are adding stuff to make it work more often, but that's not thread safety and will lead to more complex issue to debug in the futur. I would prefer that we advocate the use of ecore thread and use eina thread debugging property to prevent efl call from outside of the main loop by either asserting, displaying a backtrace or just spanking (stuff that could turned on and off at compile time and help provide development build or production build). I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). I agree with Cedric to disagree, purely on principle. Er, meaning I disagree with this patch. the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. That's the slippery slope that lead to most of todays software being so bloated. /me stops his early morning rant and gets breakfast. OK, now that I'm awake, and had brekky... Ah, thread SAFETY. Some amount of thread safety is OK so long as it does not impact on the other core values in general. Speed and lightness should be the hallmarks of EFL. So long as those are achieved, and it just happens to be thread safe to, then all is good. Still should just tell people that thread safety is not guaranteed, and hardly needed. -- A big old stinking pile of genius that no one wants coz there are too many silver coated monkeys in the world. signature.asc Description: PGP signature -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
GRUMBL !!! On Thu, Jul 28, 2011 at 1:13 PM, Mike McCormack mj.mccorm...@samsung.com wrote: On 07/28/2011 07:32 PM, Cedric BAIL wrote: Maybe stable, but the rendered scene will not make sense and be highly difficult to debug. So spank and point people to the right solution. This is ecore, not evas. Adding timers from a thread makes sense to me. I'm not proposing this as a solution for Evas. Perhaps adding a thread check is better there... I haven't looked at it closely. * nobody complained when I added checks to show that ecore calls are coming from the same thread, even though the performance impact would be comparable. (See ECORE_MAIN_LOOP_ASSERT) Because if needed, we can turn it off without breaking application or any behaviour. Something that was working with it on will work with it off. It something that we could use in a development build and remove from the production build without the need to worry about it at all. If you're worried about this, how about I add a function call that can set the _ecore_lock() and _ecore_unlock() functions. If they are NULL, the won't be called. Also, if you worry so much about performance, please let me know what benchmarking you want done to show the overhead. Before providing a benchmark, please revert your broken code and provide a working solution based on my discussion with gustavo (using Ecore_Pipe). Right know, and before doing benchmark, your solution doesn't work in all case and that's a no go for me. It's already painfull for us when we use thread that I don't want to dig why some user have weird behaviour due to this half working solution. I thing that this will get confusing and people will introduce more bug, because the barrier with thread safety will be blured, some function call will work, some won't. At the end, I think you are increasing the complexity of the EFL without any benefit, you will have more and more complex code to debug (because every thing will not be thread safe). I really am more for a macro that display a spank with backtrace and do nothing in all efl function call (including evas, edje and elementary), that point to Ecore_Thread documentation. In fact halt of that code is already provided by eina, I can provide it quickly if that feat your need. This will put the burden of the fix on the developer and not you. They will know exactly where and how to fix it. I have already added code to print warnings. It helps, but the response is usually, but why isn't EFL thread safe? Because it's not Java and most people don't understand thread. From my own experience what ever you try to do, people will mess with thread. Not a single student coming out of school will get thread right and it will take them a lot of time before they can effectivly use it safely and in a more efficient way than just using a callback/state machine as the ecore main loop. That's why we have Ecore_Thread, to put a clear barier where you use thread only for your calc and blocking operation and go back into the main loop for any UI related operation. It's here to help every developper including us. Adding thread safety means: * one whole class of API misuse and potential problems is gone And one whole class of new bug to dig in harder is in. * people who think they know what they're doing can use threads without worrying that ecore is a source of problems If they know thread, then ecore wouldn't be the source of problems. * we offer the same features to 3rd parties as we want to use inside EFL. It's a bit hypocritical to say Well, the EVAS code can use threads, because we're E-lite developers, but users of our API should never be able to use threads, and should get spanked for doing so. No it's not. Maybe we're E-lite developer, but our thread code is starting to be fine only since a few month (and i wouldn't bet on it). That's why we provide infra with Ecore_Thread to minimize the risk of race condition and other nice dead lock. It's also why we will certainly use Ecore to do the sharing ressource daemon for next generation of pipeline rendering, to avoid going into the async mess again. And I agree, we provide the same infra for 3rd parties as we want to use inside EFL, that's called Ecore_Thread. To be really clear from my point of view, thread should only be used for blocking operation and heavy computational task. Trying to advertise more than that will just end with more bug to fix in user application. Some time, it's better not to provide to much, just to be sure that most people take the wrong path. As a side note, when I want student to hit a wall, I just lure them into using thread, none of them ever get anything working properly with it in any sane amount of time. I personnally thing that Grand Central Dispatch from Apple is the right design when we are speaking about thread (http://en.wikipedia.org/wiki/Grand_Central_Dispatch) and that the Java approach to make everything thread safe
Re: [E-devel] [PATCH] ecore: Add basic thread safety
As I don't want the grumpy guy that don't help. You now have ecore_main_loop_thread_safe_call that should be the base for your patch. And I am still not convinced that we need more than that. People should just use that function when they are doing thread stuff and we should advertise it in our thread spanking message. On Thu, Jul 28, 2011 at 2:31 PM, Cedric BAIL cedric.b...@free.fr wrote: GRUMBL !!! On Thu, Jul 28, 2011 at 1:13 PM, Mike McCormack mj.mccorm...@samsung.com wrote: On 07/28/2011 07:32 PM, Cedric BAIL wrote: Maybe stable, but the rendered scene will not make sense and be highly difficult to debug. So spank and point people to the right solution. This is ecore, not evas. Adding timers from a thread makes sense to me. I'm not proposing this as a solution for Evas. Perhaps adding a thread check is better there... I haven't looked at it closely. * nobody complained when I added checks to show that ecore calls are coming from the same thread, even though the performance impact would be comparable. (See ECORE_MAIN_LOOP_ASSERT) Because if needed, we can turn it off without breaking application or any behaviour. Something that was working with it on will work with it off. It something that we could use in a development build and remove from the production build without the need to worry about it at all. If you're worried about this, how about I add a function call that can set the _ecore_lock() and _ecore_unlock() functions. If they are NULL, the won't be called. Also, if you worry so much about performance, please let me know what benchmarking you want done to show the overhead. Before providing a benchmark, please revert your broken code and provide a working solution based on my discussion with gustavo (using Ecore_Pipe). Right know, and before doing benchmark, your solution doesn't work in all case and that's a no go for me. It's already painfull for us when we use thread that I don't want to dig why some user have weird behaviour due to this half working solution. I thing that this will get confusing and people will introduce more bug, because the barrier with thread safety will be blured, some function call will work, some won't. At the end, I think you are increasing the complexity of the EFL without any benefit, you will have more and more complex code to debug (because every thing will not be thread safe). I really am more for a macro that display a spank with backtrace and do nothing in all efl function call (including evas, edje and elementary), that point to Ecore_Thread documentation. In fact halt of that code is already provided by eina, I can provide it quickly if that feat your need. This will put the burden of the fix on the developer and not you. They will know exactly where and how to fix it. I have already added code to print warnings. It helps, but the response is usually, but why isn't EFL thread safe? Because it's not Java and most people don't understand thread. From my own experience what ever you try to do, people will mess with thread. Not a single student coming out of school will get thread right and it will take them a lot of time before they can effectivly use it safely and in a more efficient way than just using a callback/state machine as the ecore main loop. That's why we have Ecore_Thread, to put a clear barier where you use thread only for your calc and blocking operation and go back into the main loop for any UI related operation. It's here to help every developper including us. Adding thread safety means: * one whole class of API misuse and potential problems is gone And one whole class of new bug to dig in harder is in. * people who think they know what they're doing can use threads without worrying that ecore is a source of problems If they know thread, then ecore wouldn't be the source of problems. * we offer the same features to 3rd parties as we want to use inside EFL. It's a bit hypocritical to say Well, the EVAS code can use threads, because we're E-lite developers, but users of our API should never be able to use threads, and should get spanked for doing so. No it's not. Maybe we're E-lite developer, but our thread code is starting to be fine only since a few month (and i wouldn't bet on it). That's why we provide infra with Ecore_Thread to minimize the risk of race condition and other nice dead lock. It's also why we will certainly use Ecore to do the sharing ressource daemon for next generation of pipeline rendering, to avoid going into the async mess again. And I agree, we provide the same infra for 3rd parties as we want to use inside EFL, that's called Ecore_Thread. To be really clear from my point of view, thread should only be used for blocking operation and heavy computational task. Trying to advertise more than that will just end with more bug to fix in user application. Some time, it's better not to provide to much, just to be sure that most
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On 07/28/2011 08:18 PM, Vincent Torri wrote: I have already added code to print warnings. It helps, but the response is usually, but why isn't EFL thread safe? tell them to read the preamble there : http://docs.enlightenment.org/auto/ecore/Ecore_Main_Loop_Page.html Well, after repeating myself N times, it's reasonable to wonder whether we're violating the principle of least surprise, then go fix/remove the surprise. If you want me to run benchmarks to show performance impact of the change, let me know which ones will be appropriate. thanks, Mike -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On 07/28/2011 09:31 PM, Cedric BAIL wrote: Before providing a benchmark, please revert your broken code and provide a working solution based on my discussion with gustavo (using Ecore_Pipe). Right know, and before doing benchmark, your solution doesn't work in all case and that's a no go for me. It's already painfull for us when we use thread that I don't want to dig why some user have weird behaviour due to this half working solution. Hi Cedric, I haven't gotten any mail to Gustavo from you. Please resend it. What is broken? Which case does it not work for? My code adds a single mutex, which only ecore can see. The rules for use are fairly simple: * lock the mutex on entering ecore code * unlock the mutex on leaving ecore code * don't call functions that lock the mutex from within ecore Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. thanks, Mike -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On 07/28/2011 10:35 PM, Cedric BAIL wrote:
As I don't want the grumpy guy that don't help. You now have
ecore_main_loop_thread_safe_call that should be the base for your
patch. And I am still not convinced that we need more than that.
People should just use that function when they are doing thread stuff
and we should advertise it in our thread spanking message.
OK, thanks for spending the time to write this.
If you know you're writing code that's in a thread your function is
great. Call it, and then schedule some work in the main loop, it's
good.
But for a library writer:
1) assume thread safety (1 line):
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
2) Using ecore_main_loop_thread_safe_call (5 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
/* will always be marshalled through pipe, even when not necessary */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
3) Using ecore_main_loop_thread_safe_call, trying to be a bit more
efficient (8 lines)
static void _event_main_loop_callback(void *data)
{
ecore_event_add(LIB_EVENT, e, my_event_free, NULL);
}
if (!ecore_is_main_loop_thread()) /* assuming this function exists */
ecore_main_loop_thread_safe_call(_event_main_loop_callback, NULL);
else
_event_main_loop_callback();
Case 2 3 are more verbose, and you need to add such code for *every*
call to ecore (in a library).
So you end up pushing responsibilities to the library writers and
applications that are better handled in Ecore itself.
IOW, it makes writing ecore code with threads a pain in the butt,
and reduces the potential audience of EFL.
thanks,
Mike
--
Got Input? Slashdot Needs You.
Take our quick survey online. Come on, we don't ask for help often.
Plus, you'll get a chance to win $100 to spend on ThinkGeek.
http://p.sf.net/sfu/slashdot-survey
___
enlightenment-devel mailing list
enlightenment-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Fri, 29 Jul 2011, Mike McCormack wrote: On 07/28/2011 09:31 PM, Cedric BAIL wrote: Before providing a benchmark, please revert your broken code and provide a working solution based on my discussion with gustavo (using Ecore_Pipe). Right know, and before doing benchmark, your solution doesn't work in all case and that's a no go for me. It's already painfull for us when we use thread that I don't want to dig why some user have weird behaviour due to this half working solution. Hi Cedric, I haven't gotten any mail to Gustavo from you. Please resend it. What is broken? Which case does it not work for? My code adds a single mutex, which only ecore can see. The rules for use are fairly simple: * lock the mutex on entering ecore code * unlock the mutex on leaving ecore code * don't call functions that lock the mutex from within ecore Secondly, this code is disabled, so it shouldn't affect anything right now. As I understand, it's normal in EFL development to add code that is disabled, debug it, then enable it later when it works. no. It's not normal at all. Vincent -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
Hi, On Wed, Jul 27, 2011 at 1:17 PM, Mike McCormack mj.mccorm...@samsung.com wrote: This patch adds some level of thread safety to ecore. It does not add thread awareness (i.e. adding a timer from a thread will not wake a sleeping main loop). So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. In my experience, developers either are ignorant of the problems of threads, or expect libraries to magically work with threads. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? I understand that some people think that thread safety is not necessary, but IMO, the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not possible, you are adding stuff to make it work more often, but that's not thread safety and will lead to more complex issue to debug in the futur. I would prefer that we advocate the use of ecore thread and use eina thread debugging property to prevent efl call from outside of the main loop by either asserting, displaying a backtrace or just spanking (stuff that could turned on and off at compile time and help provide development build or production build). I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). -- Cedric BAIL -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Wed, Jul 27, 2011 at 10:08 AM, Cedric BAIL cedric.b...@free.fr wrote: Hi, On Wed, Jul 27, 2011 at 1:17 PM, Mike McCormack mj.mccorm...@samsung.com wrote: This patch adds some level of thread safety to ecore. It does not add thread awareness (i.e. adding a timer from a thread will not wake a sleeping main loop). So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. In my experience, developers either are ignorant of the problems of threads, or expect libraries to magically work with threads. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? I understand that some people think that thread safety is not necessary, but IMO, the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not possible, you are adding stuff to make it work more often, but that's not thread safety and will lead to more complex issue to debug in the futur. I would prefer that we advocate the use of ecore thread and use eina thread debugging property to prevent efl call from outside of the main loop by either asserting, displaying a backtrace or just spanking (stuff that could turned on and off at compile time and help provide development build or production build). I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). Cedric, I disagree with you and agree with Mike. Let's face it, we pushed threads away for years, it did not help. People won't learn (precious time) and they'll just use it incorrectly or get away from EFL at all. Mike is only trying to minimize parts of the problem... and in an real world application there should be enough activity on the main thread to make this problem not happen at all. Alternatively we can have a FD on own own and write to it whenever we added stuff from worker threads. All in all I'd use the Evas rewrite Raster wants to do and think about it as well. I know it's a tremendous effort, but likely if we design things like that we can achieve always thread safe GUI. I'm not talking about locking all the paths and render immediately, we can take a simpler path to serialize access to each object and do the actual work from other threads. IMO the perfect solution would be: - PROCESS: image loading process, no loading would be done in the application process. Data would always be shared with shmem - THREAD: garbage collector defragmenting for our datatypes. - THREAD: evas rendering - THREAD: ecore mainloop Given N applications we'd account as: (1 + N * 3) threads. Given stupidly cheap Linux threads this is not a performance impact. The benefits are clear: - application would never block to users - image data would be shared among applications (themes, etc) - we'd avoid memory fragmentation and most of the leaks The downside is that the whole EFL needs to be rewritten... YET AGAIN :-D -- Gustavo Sverzut Barbieri http://profusion.mobi embedded systems -- MSN: barbi...@gmail.com Skype: gsbarbieri Mobile: +55 (19) 9225-2202 -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On Wed, 27 Jul 2011 15:08:21 +0200 Cedric BAIL cedric.b...@free.fr wrote: On Wed, Jul 27, 2011 at 1:17 PM, Mike McCormack mj.mccorm...@samsung.com wrote: This patch adds some level of thread safety to ecore. It does not add thread awareness (i.e. adding a timer from a thread will not wake a sleeping main loop). So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. In my experience, developers either are ignorant of the problems of threads, or expect libraries to magically work with threads. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? I understand that some people think that thread safety is not necessary, but IMO, the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not possible, you are adding stuff to make it work more often, but that's not thread safety and will lead to more complex issue to debug in the futur. I would prefer that we advocate the use of ecore thread and use eina thread debugging property to prevent efl call from outside of the main loop by either asserting, displaying a backtrace or just spanking (stuff that could turned on and off at compile time and help provide development build or production build). I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). I agree with Cedric to disagree, purely on principle. Er, meaning I disagree with this patch. the performance cost and complexity is easily outweighed by the benefit of meeting developer expectation. That's the slippery slope that lead to most of todays software being so bloated. /me stops his early morning rant and gets breakfast. -- A big old stinking pile of genius that no one wants coz there are too many silver coated monkeys in the world. signature.asc Description: PGP signature -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
Re: [E-devel] [PATCH] ecore: Add basic thread safety
On 07/27/2011 10:08 PM, Cedric BAIL wrote: So it will most of the time work, but in some racy case, not. Sounds to me like this doesn't change much from the current behaviour. I agree, it will work more often than previously, but still hidding bug until it's to late. It is a change it goal. The goal will be to make ecore calls thread safe, and any non-thread safe behavior is a bug. People should never use things they don't understand... and so few people understand threads. Just one question, do they use ecore_thread or there own stuff ? I disagree with people should never use things they don't understand. The whole point of a library or any other programming abstraction is to hide details and save people reimplementing these things over and over themselves. Some developers in Samsung use ecore_thread, and some don't. Honestly, ECORE_MAIN_LOOP_ASSERT has shown up quite a few problems with code. Though the majority of developers understand that EFL code should be single threaded, there are still many crashes due to threading issues. I think EFL will be seen as more stable if it is thread safe... thus these patches. You can't make it work sanely, how are you planning to synchronize the rendering state with your request from a thread. It's just not Let's stick to technical stuff rather than abstract ideas like sanity. I'm only talking about ecore here. Step by step... (Raster has big plans for the rendering engine involving threads.) If the code is lacking in one area or the other, please point it out. (Actually, some big bits, like locks around the idlers are missing from the patch I sent.) I clearly disagree on that patch going in (and I am still not discussing the issue of performance here). Well three points: * nobody complained when I added checks to show that ecore calls are coming from the same thread, even though the performance impact would be comparable. (See ECORE_MAIN_LOOP_ASSERT) * given that SMP is widely available these days, I would think that enabling simple thread based programming could dramatically improve performance of some code. * I will provide a way to switch off thread safety at build time Right now my thoughts are: * support single main loop only * add support for waking the main loop when timers, fds, etc. are added thanks, Mike -- Got Input? Slashdot Needs You. Take our quick survey online. Come on, we don't ask for help often. Plus, you'll get a chance to win $100 to spend on ThinkGeek. http://p.sf.net/sfu/slashdot-survey ___ enlightenment-devel mailing list enlightenment-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/enlightenment-devel
