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

[YUNIKORN-1881] Improve the performance of tryPlaceholderAllocate #597

Closed
wants to merge 3 commits into from

Conversation

pbacsko
Copy link
Contributor

@pbacsko pbacsko commented Jul 26, 2023

What is this PR for?

tryPlaceholderAllocate() is too expensive. It copies and sorts apps and tries to find schedulable tasks even if we do not have placeholders at all. Applications should track if they have an active placeholder allocation to avoid unnecessary copy/sorting.

What type of PR is it?

  • - Bug Fix
  • - Improvement
  • - Feature
  • - Documentation
  • - Hot Fix
  • - Refactoring

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-1881

How should this be tested?

Screenshots (if appropriate)

Questions:

  • - The licenses files need update.
  • - There is breaking changes for older versions.
  • - It needs documentation.

@pbacsko pbacsko self-assigned this Jul 26, 2023
@pbacsko pbacsko marked this pull request as draft July 26, 2023 16:57
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #597 (1ab62ad) into master (068198a) will increase coverage by 0.05%.
Report is 1 commits behind head on master.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##           master     #597      +/-   ##
==========================================
+ Coverage   77.49%   77.55%   +0.05%     
==========================================
  Files          77       77              
  Lines       12586    12687     +101     
==========================================
+ Hits         9754     9839      +85     
- Misses       2518     2533      +15     
- Partials      314      315       +1     
Files Changed Coverage Δ
pkg/scheduler/objects/queue.go 77.22% <85.71%> (+0.15%) ⬆️
pkg/scheduler/objects/application.go 68.17% <100.00%> (+0.13%) ⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pbacsko pbacsko force-pushed the YUNIKORN-1881 branch 3 times, most recently from b746c12 to bb45b3f Compare July 26, 2023 17:39
@pbacsko pbacsko marked this pull request as ready for review July 26, 2023 18:15
craigcondit
craigcondit previously approved these changes Jul 26, 2023
Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

Looks ok I think. Would like @wilfred-s to take a look as well. +1.

pkg/scheduler/objects/application.go Outdated Show resolved Hide resolved
pkg/scheduler/objects/application.go Outdated Show resolved Hide resolved
pkg/scheduler/objects/application.go Outdated Show resolved Hide resolved
pkg/scheduler/objects/application.go Outdated Show resolved Hide resolved
Copy link
Contributor

@manirajv06 manirajv06 left a comment

Choose a reason for hiding this comment

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

Few more observations, but not exactly specific to this pr.

  1. Some checks are repeated in mutiple try...() methods in queue. For example, app.IsAccepted() and sq.canRunApp()
  2. Some checks can be carried out much more earlier in queue itself instead of doing in application. For example, queue and user headroom checks.

I think these checks can be kept in one single place.

@pbacsko pbacsko force-pushed the YUNIKORN-1881 branch 2 times, most recently from d585901 to ddef037 Compare July 27, 2023 14:41
Copy link
Contributor

@wilfred-s wilfred-s left a comment

Choose a reason for hiding this comment

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

LGTM

@pbacsko pbacsko closed this in 0b3fc07 Jul 28, 2023
@pbacsko pbacsko deleted the YUNIKORN-1881 branch December 7, 2023 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants