You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "luis4a0 (via GitHub)" <gi...@apache.org> on 2026/05/13 07:25:25 UTC
[PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
luis4a0 opened a new pull request, #12083:
URL: https://github.com/apache/gluten/pull/12083
## What changes are proposed in this pull request?
The `SCOPED_TIMER` macro in `cpp/velox/utils/Common.h` wraps its
`DeltaCpuWallTimer` in a `do { ... } while (0)` block. The
`DeltaCpuWallTimer` is constructed inside the do-block, so it is also
**destroyed at the closing brace of the do-block — i.e. immediately
after construction**. The lambda fires recording essentially zero ns,
and the surrounding scope (which the call sites intend to time) is
not measured at all.
As a consequence, every existing `SCOPED_TIMER` call in the Velox
shuffle writers (`VeloxHashShuffleWriter.cc`,
`VeloxRssSortShuffleWriter.cc`) has been silently recording near-zero
values into `cpuWallTimingList_`. The per-bucket time breakdown
(Compute, FlattenRV, SplitRV, EvictPartition, Stop, …) that
`stat()` would log under `VELOX_SHUFFLE_WRITER_LOG_FLAG=1` is
unusable for performance analysis as a result.
This PR replaces the do-while wrapper with two uniquely-named local
variables that survive until the end of the **enclosing** scope —
which is what every call site already expects from a SCOPED-style
RAII timer.
```cpp
#define SCOPED_TIMER(timing) \
auto GLUTEN_SCOPED_TIMER_CONCAT(_glutenScopedTimerTarget_, __LINE__) = &(timing); \
facebook::velox::DeltaCpuWallTimer GLUTEN_SCOPED_TIMER_CONCAT(_glutenScopedTimer_, __LINE__) { \
[_glutenScopedTimerTargetPtr = GLUTEN_SCOPED_TIMER_CONCAT(_glutenScopedTimerTarget_, __LINE__)]( \
const facebook::velox::CpuWallTiming& delta) { _glutenScopedTimerTargetPtr->add(delta); } \
}
```
A few implementation notes:
- The unique-naming pattern (using `__LINE__`) is needed so that
multiple `SCOPED_TIMER` calls in the same scope don't shadow each
other.
- The extra local for the timing target (a pointer) is needed because
the `timing` macro argument may be an arbitrary expression
(e.g. `cpuWallTimingList_[CpuWallTimingCompute]`) that is not a
valid lambda capture name. Capturing a pointer to it sidesteps that.
## How was this patch tested?
I verified the change compiles cleanly across all 18 call sites of
`SCOPED_TIMER` in
`cpp/velox/shuffle/Velox{Hash,RssSort}ShuffleWriter.cc`. No new tests
are added in this PR — adding a unit test that asserts a `SCOPED_TIMER`
records non-zero elapsed time after a deliberate sleep would be a
useful defensive addition; happy to add it as a follow-up if the
reviewers want it in this PR.
End-to-end runtime validation (running TPC-DS with
`VELOX_SHUFFLE_WRITER_LOG_FLAG=1` and confirming the per-bucket
numbers are now non-zero) was not possible in my development
environment because a system-wide gluten-velox JAR took precedence
over the locally-built OSS bundle on the JNI symbol resolution path,
shadowing the patched `libvelox.so`. The fix is small and
inspection-correct against the SCOPED_TIMER pattern used elsewhere in
Velox/Folly.
Marked as draft for an extra round of eyes on the macro before flipping to ready.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12083:
URL: https://github.com/apache/gluten/pull/12083#discussion_r3235446927
##########
cpp/velox/utils/Common.h:
##########
@@ -35,11 +35,27 @@ static inline void fastCopy(void* dst, const void* src, size_t n) {
facebook::velox::simd::memcpy(dst, src, n);
}
-#define SCOPED_TIMER(timing) \
- do { \
- auto ptiming = &timing; \
- facebook::velox::DeltaCpuWallTimer timer{ \
- [ptiming](const facebook::velox::CpuWallTiming& delta) { ptiming->add(delta); }}; \
- } while (0)
+// SCOPED_TIMER — RAII-style timer that lives until the end of the
+// enclosing scope and reports elapsed CPU+wall time to `timing` on
+// destruction.
+//
+// Implementation note: a `do { … } while (0)` wrapper would destroy
+// the DeltaCpuWallTimer immediately at the end of the do-block,
+// recording essentially zero. Use uniquely-named local variables at
+// the call site so the timer survives until the enclosing scope
+// closes (which is what every existing call site already expects).
+//
+// Note: we capture the timing target by pointer in a unique local
+// variable rather than directly in the lambda, because `timing` may
+// be an arbitrary expression (e.g. `array_[idx]`) that is not a valid
+// lambda capture name.
+#define GLUTEN_SCOPED_TIMER_CONCAT_INNER(a, b) a##b
+#define GLUTEN_SCOPED_TIMER_CONCAT(a, b) GLUTEN_SCOPED_TIMER_CONCAT_INNER(a, b)
Review Comment:
Done in 771d0e0. Promoted `GLUTEN_CONCAT` in `core/utils/Macros.h` to a two-level macro — the existing form was single-level (`GLUTEN_CONCAT(foo, __LINE__)` would have expanded to literal `foo__LINE__` since `##` suppresses argument expansion), but it had zero callers in the cpp/ tree, so the upgrade is backward-compatible. Style mirrors the existing `GLUTEN_STRINGIFY` / `GLUTEN_TOSTRING` pair just above it. Per-file helpers removed from `Common.h`, which now `#include`s `utils/Macros.h` and uses `GLUTEN_CONCAT` directly. `ScopedTimerTest` continues to pass with identical timing profile. Thanks for the catch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on code in PR #12083:
URL: https://github.com/apache/gluten/pull/12083#discussion_r3233385942
##########
cpp/velox/utils/Common.h:
##########
@@ -35,11 +35,27 @@ static inline void fastCopy(void* dst, const void* src, size_t n) {
facebook::velox::simd::memcpy(dst, src, n);
}
-#define SCOPED_TIMER(timing) \
- do { \
- auto ptiming = &timing; \
- facebook::velox::DeltaCpuWallTimer timer{ \
- [ptiming](const facebook::velox::CpuWallTiming& delta) { ptiming->add(delta); }}; \
- } while (0)
+// SCOPED_TIMER — RAII-style timer that lives until the end of the
+// enclosing scope and reports elapsed CPU+wall time to `timing` on
+// destruction.
+//
+// Implementation note: a `do { … } while (0)` wrapper would destroy
+// the DeltaCpuWallTimer immediately at the end of the do-block,
+// recording essentially zero. Use uniquely-named local variables at
+// the call site so the timer survives until the enclosing scope
+// closes (which is what every existing call site already expects).
+//
+// Note: we capture the timing target by pointer in a unique local
+// variable rather than directly in the lambda, because `timing` may
+// be an arbitrary expression (e.g. `array_[idx]`) that is not a valid
+// lambda capture name.
+#define GLUTEN_SCOPED_TIMER_CONCAT_INNER(a, b) a##b
+#define GLUTEN_SCOPED_TIMER_CONCAT(a, b) GLUTEN_SCOPED_TIMER_CONCAT_INNER(a, b)
Review Comment:
This is a general macro. Can you use `GLUTEN_CONCAT` in `core/utils/Macros.h`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
Posted by "baibaichen (via GitHub)" <gi...@apache.org>.
baibaichen commented on PR #12083:
URL: https://github.com/apache/gluten/pull/12083#issuecomment-4438903630
@marin-ma pelease review it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma merged PR #12083:
URL: https://github.com/apache/gluten/pull/12083
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
Posted by "marin-ma (via GitHub)" <gi...@apache.org>.
marin-ma commented on PR #12083:
URL: https://github.com/apache/gluten/pull/12083#issuecomment-4442738114
@luis4a0 LGTM. I see this PR is marked as draft. Is it ready for review now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [VL] Fix SCOPED_TIMER macro destroying timer immediately [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on PR #12083:
URL: https://github.com/apache/gluten/pull/12083#issuecomment-4449437811
> @luis4a0 LGTM. I see this PR is marked as draft. Is it ready for review now?
Yes, I marked it as ready for review. I should do it earlier, sorry.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org