Skip to content

Commit

Permalink
Do not generate error messages in TRY_CAST and TRY(CAST) (facebookinc…
Browse files Browse the repository at this point in the history
…ubator#9900)

Summary:
Pull Request resolved: facebookincubator#9900

Generating error messages and carrying them around is quite expensive. Since we
don't use error messages in TRY_CAST or TRY(CAST) we can avoid generating them
altogether. Doing so improves performance up to 3x.

Introduce thread-local flag to indicate whether no-throw APIs need to provide
detailed error message. Use this flag to avoid generating error messages in CAST.

try_cast_invalid_nan benchmark improved 3x:

Before:

```
============================================================================
[...]hmarks/ExpressionBenchmarkBuilder.cpp     relative  time/iter   iters/s
============================================================================
cast##try_cast_invalid_empty_input                          2.21ms    452.57
cast##tryexpr_cast_invalid_empty_input                     24.23ms     41.27
cast##try_cast_invalid_nan                                 16.95ms     59.00
cast##tryexpr_cast_invalid_nan                             42.26ms     23.66
----------------------------------------------------------------------------
```

After:

```
============================================================================
[...]hmarks/ExpressionBenchmarkBuilder.cpp     relative  time/iter   iters/s
============================================================================
cast##try_cast_invalid_empty_input                          2.41ms    415.65
cast##tryexpr_cast_invalid_empty_input                     24.90ms     40.16
cast##try_cast_invalid_nan                                  5.76ms    173.52
cast##tryexpr_cast_invalid_nan                             27.75ms     36.03
```

Reviewed By: xiaoxmeng, Yuhta

Differential Revision: D57700900

fbshipit-source-id: c95b293986f2f293a33e199725ca5c50c84e415e
  • Loading branch information
mbasmanova authored and facebook-github-bot committed May 23, 2024
1 parent bd7a984 commit 63b9751
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 0 deletions.
5 changes: 5 additions & 0 deletions velox/common/base/VeloxException.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ int64_t& threadNumVeloxThrow() {
return numThrow;
}

bool& threadSkipErrorDetails() {
thread_local bool skipErrorDetails{false};
return skipErrorDetails;
}

ExceptionContext& getExceptionContext() {
thread_local ExceptionContext context;
return context;
Expand Down
19 changes: 19 additions & 0 deletions velox/common/base/VeloxException.h
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,25 @@ class VeloxRuntimeError final : public VeloxException {
/// Returns a reference to a thread level counter of Velox error throws.
int64_t& threadNumVeloxThrow();

/// Returns a reference to a thread level boolean that controls whether no-throw
/// APIs include detailed error messages in Status.
bool& threadSkipErrorDetails();

class ScopedThreadSkipErrorDetails {
public:
ScopedThreadSkipErrorDetails(bool skip = true)
: original_{threadSkipErrorDetails()} {
threadSkipErrorDetails() = skip;
}

~ScopedThreadSkipErrorDetails() {
threadSkipErrorDetails() = original_;
}

private:
bool original_;
};

/// Holds a pointer to a function that provides addition context to be
/// added to the detailed error message in case of an exception.
struct ExceptionContext {
Expand Down
3 changes: 3 additions & 0 deletions velox/expression/CastExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -920,6 +920,9 @@ void CastExpr::evalSpecialForm(
ScopedVarSetter holder{context.mutableThrowOnError(), false};
ScopedVarSetter captureErrorDetails(
context.mutableCaptureErrorDetails(), false);

ScopedThreadSkipErrorDetails skipErrorDetails(true);

apply(rows, input, context, fromType, toType, result);
} else {
apply(rows, input, context, fromType, toType, result);
Expand Down
4 changes: 4 additions & 0 deletions velox/expression/TryExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ void TryExpr::evalSpecialForm(
ScopedVarSetter captureErrorDetails(
context.mutableCaptureErrorDetails(), false);

ScopedThreadSkipErrorDetails skipErrorDetails(true);

// It's possible with nested TRY expressions that some rows already threw
// exceptions in earlier expressions that haven't been handled yet. To avoid
// incorrectly handling them here, store those errors and temporarily reset
Expand All @@ -48,6 +50,8 @@ void TryExpr::evalSpecialFormSimplified(
ScopedVarSetter captureErrorDetails(
context.mutableCaptureErrorDetails(), false);

ScopedThreadSkipErrorDetails skipErrorDetails(true);

// It's possible with nested TRY expressions that some rows already threw
// exceptions in earlier expressions that haven't been handled yet. To avoid
// incorrectly handling them here, store those errors and temporarily reset
Expand Down
3 changes: 3 additions & 0 deletions velox/type/Conversions.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ template <typename T, typename F>
Expected<T> callFollyTo(const F& v) {
const auto result = folly::tryTo<T>(v);
if (result.hasError()) {
if (threadSkipErrorDetails()) {
return folly::makeUnexpected(Status::UserError(""));
}
return folly::makeUnexpected(Status::UserError(
"{}", folly::makeConversionError(result.error(), "").what()));
}
Expand Down

0 comments on commit 63b9751

Please sign in to comment.