[C++] Excessive stack consumption with ARROW_ASSIGN_OR_RAISE

[pitrou]

Describe the bug, including details regarding any error messages, version, and platform.

While investigating some of the CI failures in #51758, it came to me that each use of ARROW_ASSIGN_OR_RAISE added stack consumption through an additional uniquely-named local variable to hold the temporary Result<T>.

A lexical sequence of ARROW_ASSIGN_OR_RAISE will therefore consume N times the necessary stack space, as this sample code shows:

using BigObject = std::array<uint8_t, 128>;

ARROW_NOINLINE Result<BigObject> ReturnSomethingBig() { return BigObject{}; }

ARROW_NOINLINE Status PrintStack(int depth = 0, uint8_t* last_stack = nullptr) {
  uint8_t x;
  if (last_stack) {
    ARROW_LOG(INFO) << "Depth " << depth << " -> stack: " << (void*)(&x)
                    << " -> frame size: " << last_stack - &x;
  } else {
    ARROW_LOG(INFO) << "Depth " << depth << " -> stack: " << (void*)(&x);
  }
  ARROW_ASSIGN_OR_RAISE(auto obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_ASSIGN_OR_RAISE(obj, ReturnSomethingBig());
  ARROW_UNUSED(obj);
  if (depth < 10) {
    RETURN_NOT_OK(PrintStack(depth + 1, &x));
  }
  return Status::OK();
}

TEST(ResultTest, StackConsumption) { ASSERT_OK(PrintStack()); }

On my machine in debug mode this prints:

[ RUN      ] ResultTest.StackConsumption
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:842: Depth 0 -> stack: 0x7ffc1d874bdf
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 1 -> stack: 0x7ffc1d87455f -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 2 -> stack: 0x7ffc1d873edf -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 3 -> stack: 0x7ffc1d87385f -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 4 -> stack: 0x7ffc1d8731df -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 5 -> stack: 0x7ffc1d872b5f -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 6 -> stack: 0x7ffc1d8724df -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 7 -> stack: 0x7ffc1d871e5f -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 8 -> stack: 0x7ffc1d8717df -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 9 -> stack: 0x7ffc1d87115f -> frame size: 1664
/home/antoine/arrow/dev/cpp/src/arrow/result_test.cc:839: Depth 10 -> stack: 0x7ffc1d870adf -> frame size: 1664

Ideally the frame size should be not much more than 128 bytes in this example.

This matters when doing recursive calls, because stack consumption can be much higher than expected.

Component(s)

C++

[pitrou]

cc @taepper

[taepper]

Uh, this turns out to be a lot trickier than I thought. The constraints that:

• lhs can be a declaration
• on error, we need to return without producing a result (might be non default-constructible)
• the intermediate Result<..> should not outlive the statement
  are extremely difficult to fulfil.

Also, you only mentioned the debug stack sizes, but this is also the case for release builds. Tests of frame sizes in bytes for 8 ARROW_ASSIGN_OR_RAISE of 128 bytes:

gcc -O0    1520
gcc -O2    1248
clang -O0  2360
clang -O2  1096

I started with a lambda version:

#define ARROW_ASSIGN_OR_RAISE(lhs, rexpr)                                   \
  ::arrow::Status st;                                                       \
  lhs = [&]() {                                                             \
    auto&& _r = (rexpr);                                                    \
    if (ARROW_PREDICT_FALSE(!_r.ok())) {                                    \
      st = _r.status();                                                     \
      return .. // what goes here?                                          \
    }                                                                       \
    return std::move(_r).ValueUnsafe();                                     \
  }();                                                                      \
  ARROW_RETURN_IF_(!st.ok(), st, ARROW_STRINGIFY(rexpr))

but that doesn't really work unless the lhs's type is default constructible, and even then could break current call sites (it overwrites lhs on error)

A bit of back and forth with an LLM showed me that this should be possible:

#define ARROW_ASSIGN_OR_RAISE(lhs, rexpr)                             \
  lhs = __extension__({                                               \
    auto&& _r = (rexpr);                                              \
    ARROW_RETURN_IF_(!_r.ok(), _r.status(), ARROW_STRINGIFY(rexpr));  \
    std::move(_r).ValueUnsafe();                                      \
  })

but only works for gcc and clang. We could then guard the implementation and use it only with these compilters.

Do you think this is acceptable? Other than that I can currently only imagine introducing another macro that does not support declarations as the lhs

[pitrou]

Thanks for investigating. How does the gcc/clang extension-based approach fare on the given example?

[taepper]

== frame sizes in bytes (variants.cc) ==
g++      -O0 (no extra context)           A_Current=1520 B_Single=336 C_Scoped=512 D_ClangGccExtension=512 E_Lambda=400 
g++      -O0 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=1536 B_Single=336 C_Scoped=528 D_ClangGccExtension=528 E_Lambda=400 
g++      -O2 (no extra context)           A_Current=1248 B_Single=176 C_Scoped=176 D_ClangGccExtension=32 E_Lambda=256 
g++      -O2 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=1264 B_Single=192 C_Scoped=192 D_ClangGccExtension=192 E_Lambda=272 
clang++  -O0 (no extra context)           A_Current=2360 B_Single=344 C_Scoped=2488 D_ClangGccExtension=2360 E_Lambda=1336 
clang++  -O0 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=2424 B_Single=344 C_Scoped=2552 D_ClangGccExtension=2424 E_Lambda=1400 
clang++  -O2 (no extra context)           A_Current=1096 B_Single=152 C_Scoped=152 D_ClangGccExtension=152 E_Lambda=216 
clang++  -O2 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=1112 B_Single=152 C_Scoped=152 D_ClangGccExtension=152 E_Lambda=216 

B_Single and C_Scoped serve as reference points of what we could achieve. Single is a single macro invocation and Scoped manually scopes the intermediate Result

the benchmarked function

using BigObject = std::array<uint8_t, 128>;

ARROW_NOINLINE Result<BigObject> Big();

#define REP8(M)                                                          \
  M(auto obj, Big());                                                    \
  M(obj, Big()); M(obj, Big()); M(obj, Big()); M(obj, Big());            \
  M(obj, Big()); M(obj, Big()); M(obj, Big());                           \
  ARROW_UNUSED(obj);                                                     \
  return Status::OK();

Status A_Current() { REP8(ARROW_ASSIGN_OR_RAISE) }

// Reference points
Status B_Single() {
  ARROW_ASSIGN_OR_RAISE(auto obj, Big());
  ARROW_UNUSED(obj);
  return Status::OK();
}

Status C_Scoped() {
  BigObject obj;
  SCOPED(Big()) SCOPED(Big()) SCOPED(Big()) SCOPED(Big())
  SCOPED(Big()) SCOPED(Big()) SCOPED(Big()) SCOPED(Big())
  ARROW_UNUSED(obj);
  return Status::OK();
}

// The two alternative macro implementations from the comment above

Status D_ClangGccExtension() { REP8(AOR_STMTEXPR) }

Status E_Lambda() { REP8(AOR_LAMBDA) }

[pitrou]

What is SCOPED above?

[taepper]

Maybe the numbers for 16 repetitions is also interesting:

== frame sizes in bytes (variants.cc) ==
g++      -O0 (no extra context)           A_Current=2736 B_Single=336 C_Scoped=544 D_ClangGccExtension=576 E_Lambda=464 
g++      -O0 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=2752 B_Single=336 C_Scoped=560 D_ClangGccExtension=592 E_Lambda=464 
g++      -O2 (no extra context)           A_Current=2464 B_Single=176 C_Scoped=176 D_ClangGccExtension=176 E_Lambda=320 
g++      -O2 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=2480 B_Single=192 C_Scoped=192 D_ClangGccExtension=192 E_Lambda=336 
clang++  -O0 (no extra context)           A_Current=4664 B_Single=344 C_Scoped=3640 D_ClangGccExtension=4664 E_Lambda=2616 
clang++  -O0 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=4792 B_Single=344 C_Scoped=3736 D_ClangGccExtension=4792 E_Lambda=2744 
clang++  -O2 (no extra context)           A_Current=2184 B_Single=152 C_Scoped=152 D_ClangGccExtension=152 E_Lambda=280 
clang++  -O2 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=2200 B_Single=152 C_Scoped=152 D_ClangGccExtension=152 E_Lambda=280 

Also 32 repetitions for completeness:

== frame sizes in bytes (variants.cc) ==
g++      -O0 (no extra context)           A_Current=5168 B_Single=336 C_Scoped=672 D_ClangGccExtension=704 E_Lambda=592 
g++      -O0 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=5184 B_Single=336 C_Scoped=688 D_ClangGccExtension=720 E_Lambda=592 
g++      -O2 (no extra context)           A_Current=4896 B_Single=176 C_Scoped=176 D_ClangGccExtension=176 E_Lambda=608 
g++      -O2 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=4912 B_Single=192 C_Scoped=192 D_ClangGccExtension=192 E_Lambda=656 
clang++  -O0 (no extra context)           A_Current=9272 B_Single=344 C_Scoped=8248 D_ClangGccExtension=9272 E_Lambda=5176 
clang++  -O0 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=9528 B_Single=344 C_Scoped=8472 D_ClangGccExtension=9528 E_Lambda=5432 
clang++  -O2 (no extra context)           A_Current=4360 B_Single=152 C_Scoped=152 D_ClangGccExtension=152 E_Lambda=408 
clang++  -O2 -DARROW_EXTRA_ERROR_CONTEXT  A_Current=4376 B_Single=152 C_Scoped=152 D_ClangGccExtension=152 E_Lambda=408

Debug builds will still use some more memory for every additional macro invocation, but not excessive. Release builds will no longer exhibit growing frame sizes

[taepper]

SCOPED is manually scoping the call to Big() on every invocation:

#define SCOPED(rexpr)                                     \
  {                                                       \
    auto&& _r = (rexpr);                                  \
    ARROW_RETURN_IF_(!_r.ok(), _r.status(), "");          \
    obj = std::move(_r).ValueUnsafe();                    \
  }

this serves just as a reference point. We cannot use this as the declaration will not be in the current context

[taepper]

(I updated the measurements above to include clang++)

[pitrou]

Ok, I'm not sure what to do about this. We could add the gcc/clang extension specialization to solve the issue on those compiler, but it would still exist otherwise (for example with MSVC).

@zanmato1984 @HuaHuaY Thoughts?