> On Nov. 20, 2014, 10:39 p.m., Dominic Hamon wrote: > > 3rdparty/libprocess/src/libev.hpp, line 39 > > <https://reviews.apache.org/r/28184/diff/3/?file=771698#file771698line39> > > > > this makes me uncomfortable but i can't quite pin down why. > > > > would a better construction be: > > > > ThreadLocal<bool>* in_event_loop() > > { > > static ThreadLocal<bool> in_event_loop; > > return &in_event_loop; > > } > > > > which gets you lazy initialization of the thread local but without > > macro usage, and avoids the issue of the non-determinism of static > > initialization. > > Benjamin Hindman wrote: > Cool, I like this better than the macros too! But lets make the static be > a pointer so that it doesn't get cleaned up non-determinisitically at program > shutdown. Also, I don't know what non-determinism you are referring to > regarding static initialization? > > Joris: can we do this for both __executor__ and __in_event_loop__ please? > Thanks! > > Dominic Hamon wrote: > Nice catch. Yes, a pointer will be safer. > > The global ThreadLocal in the current patch is initialized with " = new > ThreadLocal...". There's no guarantee that this statement will be run before > the object is accessed when a method is called from a different translation > unit. The function static guarantees that when it is first accessed it is > initialized. > > Benjamin Hindman wrote: > After chatting more with Joris we decided that we wanted to make this > work and look more similar to how it will work in the future with C++11's new > 'thread_local'. In that case, we'll just be able to have: > > thread_local bool __in_event_loop__ = false; > > And since this is just a POD we don't need to worry about > shutdown/destructor stuff. > > Dominic Hamon wrote: > Our ThreadLocal is NOT POD. > > "A Plain Old Data Structure in C++ is an aggregate class that contains > only PODS as members, has no user-defined destructor, no user-defined copy > assignment operator, and no nonstatic members of pointer-to-member type." > > we have a user-defined destructor and a user-defined copy assignment > operator. You absolutely have to worry about shutdown/destructor stuff. > > Until we have g++-4.8 we don't have a non-POD thread_local. Please fix > this to avoid any difficult to debug issues.
I agree, ThreadLocal is not POD, but ThreadLocal* is POD and that's what we're using (which is also what we use for the __executor__ ThreadLocal). As I commented on above, I *would not* want to just use ThreadLocal<bool> as a global (or as a static variable in a function) because of the possible shutdown/destructor problems. But since we're using ThreadLocal<bool>*, we don't have to worry about that. The POD I was referring to was once we have g++-4.8 and can use thread_local. In that case, the POD will just be a 'bool', and we again won't need to worry about shutdown/destructor stuff. Thus, we tried to make this code change as little as possible between today (no thread_local) and tomorrow (yes thread_local). Of course, even with thread_local we'll still have the problem for non-POD stuff, which is why we'll always want to make them pointers (like we already do for our globals). Sorry if I wasn't clear earlier. Make sense? - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28184/#review62448 ----------------------------------------------------------- On Nov. 21, 2014, 10:18 p.m., Joris Van Remoortere wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28184/ > ----------------------------------------------------------- > > (Updated Nov. 21, 2014, 10:18 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-2125 > https://issues.apache.org/jira/browse/MESOS-2125 > > > Repository: mesos-git > > > Description > ------- > > This improves performance by short-circuiting run_in_event_loop to just call > the lambda if it is already in the event loop. > > > Diffs > ----- > > 3rdparty/libprocess/src/libev.hpp 04847e30b0d764d258b8c0971da87180043161f2 > 3rdparty/libprocess/src/process.cpp > cf75b51fdae821c9b6c80ddcf2a56ba8bbc63f14 > > Diff: https://reviews.apache.org/r/28184/diff/ > > > Testing > ------- > > make check > 3rdparty/libprocess/benchmark improved throughput ~27% > > > Thanks, > > Joris Van Remoortere > >
