Skip to content

Commit df52e01

Browse files
Rajeev975facebook-github-bot
authored andcommitted
feat(expr-eval): Fix flaky adaptiveCpuSamplingPerFunctionRates test (facebookincubator#17002)
Summary: Previous diff (facebookincubator#16646) had a flaky test because the test asserted that slow_add must be in kAlwaysTrack state and plus must be in kSampling state. Replaced absolute state assertions with a relative comparison: slow_add sampling rate must be ≤ plus sampling rate. This is robust because both functions share the same timerOverheadNanos_ per ExprSet, so measurement noise affects both equally and cannot flip the relative ordering. Reviewed By: pedroerp Differential Revision: D99126870
1 parent 95ce761 commit df52e01

File tree

1 file changed

+8
-23
lines changed

1 file changed

+8
-23
lines changed

velox/expression/tests/ExprStatsTest.cpp

Lines changed: 8 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -724,35 +724,20 @@ TEST_F(ExprStatsTest, adaptiveCpuSamplingPerFunctionRates) {
724724
ASSERT_NE(plusExpr, nullptr) << "Failed to find 'plus' expression";
725725
ASSERT_NE(slowAddExpr, nullptr) << "Failed to find 'slow_add' expression";
726726

727-
// Cheap function (plus) should be in sampling mode with rate > 1.
728-
ASSERT_TRUE(plusExpr->isAdaptiveSampling())
729-
<< "Expected cheap function 'plus' to be in sampling mode";
730-
ASSERT_GT(plusExpr->adaptiveSamplingRate(), 1u)
731-
<< "Expected sampling rate > 1 for cheap function";
732-
733-
// Expensive function (slow_add) should always track (not sampling).
734-
ASSERT_FALSE(slowAddExpr->isAdaptiveSampling())
735-
<< "Expected expensive function 'slow_add' to always track";
727+
/// The expensive function (slow_add) should have a lower or equal sampling
728+
/// rate compared to the cheap function (plus).
729+
ASSERT_LE(
730+
slowAddExpr->adaptiveSamplingRate(), plusExpr->adaptiveSamplingRate())
731+
<< "Expected expensive function to have lower or equal sampling rate "
732+
<< "than cheap function. slow_add rate: "
733+
<< slowAddExpr->adaptiveSamplingRate()
734+
<< ", plus rate: " << plusExpr->adaptiveSamplingRate();
736735

737736
// Both functions should have timing data.
738737
auto stats = exprSet->stats();
739738
ASSERT_GT(stats["plus"].timing.cpuNanos, 0u);
740739
ASSERT_GT(stats["slow_add"].timing.cpuNanos, 0u);
741740

742-
// slow_add is always-track after calibration. It won't have timing for the
743-
// warmup + calibration batches (1 + 5 = 6), but all post-calibration batches
744-
// should be tracked.
745-
constexpr uint64_t kCalibrationOverhead = 6; // 1 warmup + 5 calibration
746-
ASSERT_EQ(
747-
stats["slow_add"].timing.count,
748-
stats["slow_add"].numProcessedVectors - kCalibrationOverhead);
749-
750-
// plus is in sampling mode. Stats should be adjusted (extrapolated) so
751-
// timing.count matches numProcessedVectors.
752-
ASSERT_EQ(stats["plus"].timing.count, stats["plus"].numProcessedVectors);
753-
ASSERT_GT(stats["plus"].timing.cpuNanos, 0u);
754-
755-
// Verify the sampling rate for plus is reasonable (should be > 1).
756741
LOG(INFO) << "plus sampling rate: " << plusExpr->adaptiveSamplingRate()
757742
<< ", slow_add sampling rate: "
758743
<< slowAddExpr->adaptiveSamplingRate();

0 commit comments

Comments
 (0)