mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Review Request 18173: Updated Try to return a const reference through get() to reduce unnecessary copying.
Date Sun, 16 Feb 2014 23:38:32 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18173/
-----------------------------------------------------------

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message