Skip to content

[core] Skip placement group resource parsing when updating existing available resources - #66318

Merged
rueian merged 5 commits into
ray-project:masterfrom
dancingactor:optimize-tryallocate
Oct 22, 2026
Merged

rueian merged 5 commits into
ray-project:masterfrom
dancingactor:optimize-tryallocate

Conversation

@dancingactor

@dancingactor dancingactor commented Sep 19, 2026 •

Copy link
Copy Markdown
Contributor

Description

This PR skips placement group resource parsing when updating existing available resources.

Since this updating logic is going to be used in global scheduling (selecting a node within the cluster to assign a task) and local allocation in the upcoming PR #65871, this critical path needs to be optimized.

Performance Test

The optimization is 1.7×–24× faster on the alloc/free paths:

Scenario Before After Speedup
CPU alloc+free 4252 ns 2448 ns 1.74×
CPU alloc failure 2261 ns 2229 ns ~1.0×
GPU alloc+free (full) 4283 ns 2357 ns 1.82×
GPU alloc+free (fractional) 4294 ns 2412 ns 1.78×
GPU alloc failure 2214 ns 2306 ns ~1.0×
Free only 980 ns 41 ns 24.0×
Subtract 1016 ns 101 ns 10.1×

Related PR

Related to #65871

@dancingactor
dancingactor marked this pull request as ready for review September 20, 2026 10:47
@dancingactor
dancingactor requested a review from a team as a code owner September 20, 2026 10:47

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the resource allocation logic in NodeResourceInstanceSet::TryAllocate to optimize lookups and handle implicit resources. However, the implementation incorrectly assumes that mixed resource demands (such as 1.5 GPUs) are invalid, splitting the allocation into mutually exclusive integer and fractional branches. This introduces a critical regression where mixed requests always fail, which also led to modifying several unit tests to use integer demands instead of fractional ones. To resolve this, the allocation logic should be updated to allow mixed demands by falling through to fractional allocation after allocating full instances, and the corresponding unit tests should be reverted to their original state.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

@ray-gardener ray-gardener Bot added core Issues that should be addressed in Ray Core community-contribution Contributed by the community labels Sep 20, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit e6d5e7e. Configure here.

@Yicheng-Lu-llll Yicheng-Lu-llll self-assigned this Sep 21, 2026

@Yicheng-Lu-llll Yicheng-Lu-llll left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I suspect the major time we spend is regex(ParsePgFormattedResource) not vector copying. could you do some experiments to very that? if regex is the major cost, If, we could just skip the parse when the key is already there.

place and undoing on failure feels fragile.

Signed-off-by: dancingactor <s990346@gmail.com>
@dancingactor

dancingactor commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor Author

After verifying, your suspicion makes perfect sense. The biggest cost is regex(ParsePgFormattedResource) not vector copying, so I've gone with the Set()-only fix rather than the in-place-mutation approach. It recovers ~90–95% of the total speedup and requires no fragile undo-on-failure logic

Scenario Before (master) Set() fix only in-place only in-place + Set() fix
CPU alloc+free 4252.42 2448.00 2264.03 2302.67
CPU alloc failure 2260.87 2229.40 2085.74 2109.22
GPU alloc+free (full) 4282.83 2356.93 2242.38 2290.81
GPU alloc+free (frac) 4293.57 2412.36 2240.00 2329.91
GPU alloc failure 2213.86 2305.68 2190.77 2136.17
Free only 980.00 40.80 5.00 4.90
Subtract 1015.98 100.76 35.54 34.20

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had
any activity for 14 days. It will be closed in another 14 days if no further activity occurs.
Thank you for your contributions.

You can always ask for help on our discussion forum or Ray's public slack channel.

If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@github-actions github-actions Bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Oct 11, 2026
@github-actions github-actions Bot added unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it. and removed stale The issue is stale. It will be closed within 7 days unless there are further conversation labels Oct 13, 2026

@Yicheng-Lu-llll Yicheng-Lu-llll left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! left some nits, would be great to also update the pr body.

Signed-off-by: dancingactor <s990346@gmail.com>
Signed-off-by: dancingactor <s990346@gmail.com>
@dancingactor dancingactor changed the title [core][fractional resource] Optimize per-instance allocation logic [core] Skip placement group resource parsing when updating existing available resources Oct 16, 2026
@dancingactor

Copy link
Copy Markdown
Contributor Author

Updated the PR description. Thanks for review!

@Yicheng-Lu-llll Yicheng-Lu-llll added the go add ONLY when ready to merge, run all tests label Oct 19, 2026
@rueian
rueian enabled auto-merge (squash) October 20, 2026 18:01
@rueian
rueian merged commit f4f37d3 into ray-project:master Oct 22, 2026
7 checks passed
@dancingactor
dancingactor deleted the optimize-tryallocate branch October 28, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

community-contribution Contributed by the community core Issues that should be addressed in Ray Core go add ONLY when ready to merge, run all tests unstale A PR that has been marked unstale. It will not get marked stale again if this label is on it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants