----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18173/#review34679 -----------------------------------------------------------
Ship it! Ship It! - Niklas Nielsen On Feb. 16, 2014, 3:38 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18173/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2014, 3:38 p.m.) > > > Review request for mesos, Benjamin Hindman, Jie Yu, Niklas Nielsen, and Vinod > Kone. > > > Bugs: MESOS-1008 > https://issues.apache.org/jira/browse/MESOS-1008 > > > Repository: mesos-git > > > Description > ------- > > See MESOS-1008. > > The main goal here is to reduce copying in Try. Most of the code using 'Try's > will either make a copy from t.get(), or use t.get() repeatedly. The latter > use results in many copy operations, one for each call to get(). > > There is one misuse as a result of this change that will not get caught by > the compiler: > const X& x = try.get().x; > try = foo(); // Now x is garbage. > x.bar(); // Bad! > > Fortunately, there were no instances of this in the code. > > Similar updates to Option and Result will follow. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp > 6a56c7d175e2e1c0b5f917d8a2d92641d6b7e007 > > Diff: https://reviews.apache.org/r/18173/diff/ > > > Testing > ------- > > Updates for libprocess and mesos will come, there were no bugs as a result of > this change, rather only cleanups. > > > I added the following test to verify the reduction in copies. Interestingly, > gcc-4.8 was eliding the Try copy regardless of whether -O2 was supplied. > > I first modified the Try copy constructor: > Try(const Try<T>& that) > { > std::cerr << " Try copy" << std::endl; // Added this. > ... > } > > struct Foo > { > Foo() : a(1), b(2) {} > Foo(const Foo& f) { a = f.a; b = f.b; std::cerr << " Foo copy" << > std::endl; } > int a, b; > }; > > > Try<Foo> bar() { return Foo(); } > > > // This test captures the number of copies occurring on stderr. > TEST(TryTest, Copy) > { > Try<Foo> foo = bar(); > Foo f1 = foo.get(); > > const Foo& f2 = foo.get(); // This patch eliminates a copy here. > > std::cerr << foo.get().a << std::endl; // This patch eliminates a copy here. > std::cerr << foo.get().b << std::endl; // This patch eliminates a copy here. > } > > > Current Try: > g++-4.8 with and without O2: > Try<Foo> foo = bar(); > Foo copy > Foo f1 = foo.get(); > Foo copy > const Foo& f2 = foo.get(); > Foo copy > foo.get().a > Foo copy > 1 > foo.get().b > Foo copy > 2 > > > New Try: > g++-4.8 with and without O2: > Try<Foo> foo = bar(); > Foo copy > Foo f1 = foo.get(); > Foo copy > const Foo& f2 = foo.get(); > foo.get().a > 1 > foo.get().b > 2 > > > Thanks, > > Ben Mahler > >
