> On Nov. 20, 2014, 2: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. > > Benjamin Hindman wrote: > 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?
Yes, that does make sense. And your optimism about g++-4.8 is noted :) I am still concerned about the global pointer is initialized as I'm pretty sure any function calls from other translation units won't guarantee that the pointer has been allocated before they are called. We do this everywhere, though, so we're either very lucky or it hasn't been a problem ;) We just need to keep an eye on it and be prepared to change how we construct this pattern if a problem hits. - Dominic ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28184/#review62448 ----------------------------------------------------------- On Nov. 21, 2014, 2: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, 2: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 > >
