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.

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

Detect subNode phase updates to reduce evaluation frequency of ArrayNode #4535

Merged
merged 5 commits into from
Dec 8, 2023

Conversation

hamersaw
Copy link
Contributor

@hamersaw hamersaw commented Dec 6, 2023

Tracking issue

NA

Why are the changes needed?

Reduces the frequency of eventing and CRD updates to improve performance.

What changes were proposed in this pull request?

Rather than incrementing the TaskPhaseVersion on the ArrayNodeState during every loop we detect if any of the subNode phases or task phases have changed. If so, we increment the TaskPhaseVersion. This vastly reduces the frequency with which ArrayNodes are evaluated, therefore improving performance on large concurrency executions.

How was this patch tested?

Locally with pprof and otel.

Setup process

NA

Screenshots

NA

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

NA

Docs link

NA

@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Dec 6, 2023
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f440278) 58.92% compared to head (a2ed1a2) 58.92%.

Files Patch % Lines
...ytepropeller/pkg/controller/nodes/array/handler.go 90.00% 0 Missing and 1 partial ⚠️
flytepropeller/pkg/controller/nodes/executor.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4535   +/-   ##
=======================================
  Coverage   58.92%   58.92%           
=======================================
  Files         620      620           
  Lines       52432    52441    +9     
=======================================
+ Hits        30896    30903    +7     
- Misses      19071    19073    +2     
  Partials     2465     2465           
Flag Coverage Δ
unittests 58.92% <85.71%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@pvditt pvditt left a comment

Choose a reason for hiding this comment

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

looks great. For posterity, could you post the perf improvements summary?

@hamersaw hamersaw merged commit b50ba87 into master Dec 8, 2023
41 checks passed
@hamersaw hamersaw deleted the performance/arraynode-subnode-transition-latency branch December 8, 2023 23:37
@hamersaw
Copy link
Contributor Author

hamersaw commented Dec 8, 2023

looks great. For posterity, could you post the perf improvements summary?

Great call out. Will add in next PR for parallelizing evaluations in ArrayNode.

eapolinario added a commit that referenced this pull request Dec 12, 2023
pvditt pushed a commit that referenced this pull request Dec 13, 2023
…ode (#4535)

* detecting subNode or task phase updates to increment TaskPhaseVersion on ArrayNode state

Signed-off-by: Daniel Rammer <[email protected]>

* not writting empty file on no inputs

Signed-off-by: Daniel Rammer <[email protected]>

---------

Signed-off-by: Daniel Rammer <[email protected]>
Signed-off-by: Paul Dittamo <[email protected]>
@hamersaw hamersaw mentioned this pull request Dec 13, 2023
3 tasks
eapolinario added a commit that referenced this pull request Dec 18, 2023
…f ArrayNode (#4535)"

This reverts commit b50ba87.

Signed-off-by: Eduardo Apolinario <[email protected]>
eapolinario added a commit that referenced this pull request Dec 20, 2023
* Revert "Detect subNode phase updates to reduce evaluation frequency of ArrayNode (#4535)"

This reverts commit b50ba87.

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add flyin pflags

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add taskTemplate as parameter to GetLogsForContainerInPod

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add flyin template scheme and unit tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Revert unintended change.

Signed-off-by: Eduardo Apolinario <[email protected]>

* Lint

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix bad refactor due to lint warning

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove TODOs

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants