Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Measure isBlocked and isFinished operator calls #10923

Closed
wants to merge 1 commit into from
Closed

Measure isBlocked and isFinished operator calls #10923

wants to merge 1 commit into from

Conversation

yingsu00
Copy link
Collaborator

@yingsu00 yingsu00 commented Sep 4, 2024

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 4, 2024
@netlify Netlify
Copy link

netlify bot commented Sep 4, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit b8705e6
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/66fcded2f151a90008a65766

@yingsu00 yingsu00 force-pushed the isBlockedTime branch 2 times, most recently from 34d307d to 53f061a Compare September 20, 2024 07:42
@yingsu00 yingsu00 marked this pull request as ready for review September 21, 2024 01:06
@yingsu00
Copy link
Collaborator Author

@Yuhta @xiaoxmeng Can you please review this? Thanks!

Yuhta
Yuhta reviewed Sep 25, 2024
View reviewed changes
velox/exec/tests/TaskTest.cpp
@@ -1184,7 +1184,7 @@ DEBUG_ONLY_TEST_F(TaskTest, liveStats) {
EXPECT_EQ(32 * numBatches, operatorStats.outputBytes);
EXPECT_EQ(3 * numBatches, operatorStats.outputPositions);
EXPECT_EQ(numBatches, operatorStats.outputVectors);
EXPECT_EQ(1, operatorStats.finishTiming.count);
EXPECT_EQ(2, operatorStats.finishTiming.count);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to check isBlockedTiming e.g. in HashProbe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. I just added it to this test.

for (auto i = 0; i < numBatches; ++i) {
    ...
    EXPECT_EQ(2 * (i + 1), operatorStats.isBlockedTiming.count);

Sorry I didn't add it until now. I was blocked by the build issues for a long time.

@yingsu00 yingsu00 force-pushed the isBlockedTime branch 2 times, most recently from 9e68cc9 to 1f1285f Compare September 27, 2024 00:46
xiaoxmeng
xiaoxmeng reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yingsu00 LGTM and can you complete the test? Thanks!

xiaoxmeng
xiaoxmeng approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yingsu00 thanks!

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

kevinwilfong
kevinwilfong reviewed Sep 30, 2024
View reviewed changes
velox/exec/tests/TaskTest.cpp Outdated
@@ -1184,7 +1185,8 @@ DEBUG_ONLY_TEST_F(TaskTest, liveStats) {
EXPECT_EQ(32 * numBatches, operatorStats.outputBytes);
EXPECT_EQ(3 * numBatches, operatorStats.outputPositions);
EXPECT_EQ(numBatches, operatorStats.outputVectors);
EXPECT_EQ(1, operatorStats.finishTiming.count);
EXPECT_EQ(2 * numBatches, operatorStats.isBlockedTiming.count);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this condition is failing in the tests

@yingsu00 yingsu00 force-pushed the isBlockedTime branch 3 times, most recently from 7210e9f to b8705e6 Compare October 2, 2024 05:49
@yingsu00
Collect isBlocked and isFinished timings in OperatorStats
b8705e6 
For some operators, the isBlocked() function call is non-trivial. This
commit adds the isBlockedTiming to OperatorStats and count it in
Driver::runInternal(). It also count the isFinished() operator which
was previously missed being counted..

Note that this does not include the Presto protocol change, which will
be done later.
@facebook-github-bot
Copy link
Contributor

@xiaoxmeng has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in f4ca3c6.

@conbench-facebook conbench-facebook
Copy link

Conbench analyzed the 1 benchmark run on commit f4ca3c6f.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

@zuyu zuyu mentioned this pull request Oct 15, 2024
Merged
yingsu00 added a commit to yingsu00/velox that referenced this pull request Oct 28, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
588eec0 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
@yingsu00 yingsu00 mentioned this pull request Oct 28, 2024
Closed
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 2, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
925f460 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 6, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
474d1ed 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 13, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
dea9221 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 13, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
33b31ab 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 15, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
355fa68 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 15, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
5eb152e 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 27, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
84c3a8e 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 27, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
b84c68f 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Nov 28, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
cb369fc 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Dec 3, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
a9a5733 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
yingsu00 added a commit to yingsu00/velox that referenced this pull request Dec 3, 2024
@yingsu00
misc: Add isBlockedTiming to PlanNodeStats
58d3d32 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
facebook-github-bot pushed a commit that referenced this pull request Dec 3, 2024
@yingsu00 @facebook-github-bot
misc: Add isBlockedTiming to PlanNodeStats (#11361)
2dae23f 
Summary:
In previous commit #10923,
    we added isBlockedTiming to OperatorStats. This commit adds it to
    PlanNodeStats.

Pull Request resolved: #11361

Reviewed By: gggrace14

Differential Revision: D66692332

Pulled By: xiaoxmeng

fbshipit-source-id: b09ac85408f79c1f77b9685977f9dd696296f71a
yingsu00 added a commit to yingsu00/velox that referenced this pull request Dec 5, 2024
@yingsu00
Add isBlockedTiming to PlanNodeStats
cabadb4 
In previous commit facebookincubator#10923,
we added isBlockedTiming to OperatorStats. This commit adds it to
PlanNodeStats.
TongWei1105 pushed a commit to TongWei1105/velox that referenced this pull request Dec 21, 2024
misc: Add isBlockedTiming to PlanNodeStats (facebookincubator#11361)
bbd5725 
Summary:
In previous commit facebookincubator#10923,
    we added isBlockedTiming to OperatorStats. This commit adds it to
    PlanNodeStats.

Pull Request resolved: facebookincubator#11361

Reviewed By: gggrace14

Differential Revision: D66692332

Pulled By: xiaoxmeng

fbshipit-source-id: b09ac85408f79c1f77b9685977f9dd696296f71a
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
@yingsu00 @athmaja-n
Measure isBlocked and isFinished operator calls (facebookincubator#10923
9fd4cfc 
)

Summary: Pull Request resolved: facebookincubator#10923

Reviewed By: kevinwilfong

Differential Revision: D63659424

Pulled By: xiaoxmeng

fbshipit-source-id: 3f0081fbef6d523f8cd8bee371cf8eab82407b0a
athmaja-n pushed a commit to athmaja-n/velox that referenced this pull request Jan 10, 2025
@yingsu00 @athmaja-n
misc: Add isBlockedTiming to PlanNodeStats (facebookincubator#11361)
235056f 
Summary:
In previous commit facebookincubator#10923,
    we added isBlockedTiming to OperatorStats. This commit adds it to
    PlanNodeStats.

Pull Request resolved: facebookincubator#11361

Reviewed By: gggrace14

Differential Revision: D66692332

Pulled By: xiaoxmeng

fbshipit-source-id: b09ac85408f79c1f77b9685977f9dd696296f71a
lingbin
lingbin reviewed Jan 26, 2025
View reviewed changes
velox/exec/Driver.cpp
nextOp,
curOperatorId_ + 1,
kOpMethodIsBlocked);
withDeltaCpuWallTimer(op, &OperatorStats::isBlockedTiming, [&]() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be nextOp here, The time consumed by nextOp->isBlocked() counted here should be accumulated to nextOp, not op.

See #12173

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will see this might take some time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lingbin lingbin lingbin left review comments

@kevinwilfong kevinwilfong kevinwilfong left review comments

@Yuhta Yuhta Yuhta left review comments

@Chiassonastraypatrick Chiassonastraypatrick Chiassonastraypatrick approved these changes

@xiaoxmeng xiaoxmeng xiaoxmeng approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yingsu00 @facebook-github-bot @Yuhta @kevinwilfong @lingbin @xiaoxmeng @Chiassonastraypatrick