Expression(0.0) re-uses the pointer of Expression::Zero() (RobotLocom…

…otion#12459)

Given a scalar type T, Eigen assumes that the constructor for T(0) is
idempotent. Consequently, it should not involve memory-allocation to avoid a
memory leak. This is not a correct assumption and needs to be fixed, but it
takes time to fix the upstream and use the latest version of Eigen. Here, we
want to provide a workaround on our side.

See RobotLocomotion#12453.

common/symbolic_expression.cc

@@ -42,19 +42,6 @@ bool operator<(ExpressionKind k1, ExpressionKind k2) {
 }
 
 namespace {
-// This function is used in Expression(const double d) constructor. It turns out
-// a ternary expression "std::isnan(d) ? make_shared<ExpressionNaN>() :
-// make_shared<ExpressionConstant>()" does not work due to C++'s
-// type-system. It throws "Incompatible operand types when using ternary
-// conditional operator" error. Related S&O entry:
-// http://stackoverflow.com/questions/29842095/incompatible-operand-types-when-using-ternary-conditional-operator.
-shared_ptr<ExpressionCell> make_cell(const double d) {
-  if (std::isnan(d)) {
-    return make_shared<ExpressionNaN>();
-  }
-  return make_shared<ExpressionConstant>(d);
-}
-
 // Negates an addition expression.
 // - (E_1 + ... + E_n) => (-E_1 + ... + -E_n)
 Expression NegateAddition(const Expression& e) {
@@ -70,6 +57,20 @@ Expression NegateMultiplication(const Expression& e) {
 }
 }  // namespace
 
+shared_ptr<ExpressionCell> Expression::make_cell(const double d) {
+  if (d == 0.0) {
+    // The objects created by `Expression(0.0)` share the unique
+    // `ExpressionConstant` object created in `Expression::Zero()`.
+    //
+    // See https://github.com/RobotLocomotion/drake/issues/12453 for details.
+    return Expression::Zero().ptr_;
+  }
+  if (std::isnan(d)) {
+    return make_shared<ExpressionNaN>();
+  }
+  return make_shared<ExpressionConstant>(d);
+}
+
 Expression::Expression(const Variable& var)
     : ptr_{make_shared<ExpressionVar>(var)} {}
 Expression::Expression(const double d) : ptr_{make_cell(d)} {}
@@ -88,22 +89,26 @@ void Expression::HashAppend(DelegatingHasher* hasher) const {
 }
 
 Expression Expression::Zero() {
-  static const never_destroyed<Expression> zero{0.0};
+  static const never_destroyed<Expression> zero{
+      Expression{make_shared<ExpressionConstant>(0.0)}};
   return zero.access();
 }
 
 Expression Expression::One() {
-  static const never_destroyed<Expression> one{1.0};
+  static const never_destroyed<Expression> one{
+      Expression{make_shared<ExpressionConstant>(1.0)}};
   return one.access();
 }
 
 Expression Expression::Pi() {
-  static const never_destroyed<Expression> pi{M_PI};
+  static const never_destroyed<Expression> pi{
+      Expression{make_shared<ExpressionConstant>(M_PI)}};
   return pi.access();
 }
 
 Expression Expression::E() {
-  static const never_destroyed<Expression> e{M_E};
+  static const never_destroyed<Expression> e{
+      Expression{make_shared<ExpressionConstant>(M_E)}};
   return e.access();
 }
 

common/symbolic_expression.h

@@ -546,7 +546,11 @@ class Expression {
   friend class ExpressionPow;
 
  private:
+  // This is a helper function used to handle `Expression(double)` constructor.
+  static std::shared_ptr<ExpressionCell> make_cell(double d);
+
   explicit Expression(std::shared_ptr<ExpressionCell> ptr);
+
   void HashAppend(DelegatingHasher* hasher) const;
 
   // Note: We use "non-const" ExpressionCell type. This allows us to perform

common/test/symbolic_expression_matrix_test.cc

@@ -552,6 +552,17 @@ TEST_F(SymbolicExpressionMatrixTest, Inverse) {
                               Substitute(M, subst).inverse(), 1e-10));
 }
 
+// We found that the following example could leak memory. This test makes sure
+// that we provide a correct work-around. FYI, `--config asan` option is
+// required to see failures from this test case.
+//
+// See https://github.com/RobotLocomotion/drake/issues/12453 for details.
+TEST_F(SymbolicExpressionMatrixTest, SparseMatrixMultiplicationNoMemoryLeak) {
+  Eigen::SparseMatrix<Expression> M1(2, 2);
+  Eigen::SparseMatrix<Expression> M2(2, 2);
+  (M1 * M2).eval();
+}
+
 }  // namespace
 }  // namespace symbolic
 }  // namespace drake