Skip to content

Conversation

@frank-wei
Copy link
Contributor

@frank-wei frank-wei commented Nov 7, 2025

Summary: Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Differential Revision: D86436375

image image

Copy link
Contributor

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

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 introduces profiling scopes using record_function_or_nullcontext in key areas of the scheduler and engine core. This will help in identifying performance hotspots and analyzing cycle-heavy areas. The changes are well-targeted and correctly placed. I've found a minor typo in one of the scope names, which I've commented on. Otherwise, the changes look good.

Comment on lines 300 to 307
with record_function_or_nullcontext("LLM_ENGINE STEP:: RECORD_STATS"):
if self.logger_manager is not None and outputs.scheduler_stats is not None:
self.logger_manager.record(
scheduler_stats=outputs.scheduler_stats,
iteration_stats=iteration_stats,
mm_cache_stats=self.processor.stat_mm_cache(),
)
self.do_log_stats_with_interval()
Copy link
Contributor

Choose a reason for hiding this comment

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

high

There appears to be a typo in the profiler scope name. It uses a double colon (::) which is inconsistent with the other scopes in this file that use a single colon. For consistency and to ensure easier parsing of traces, this should be corrected to LLM_ENGINE STEP: RECORD_STATS.

Suggested change
with record_function_or_nullcontext("LLM_ENGINE STEP:: RECORD_STATS"):
if self.logger_manager is not None and outputs.scheduler_stats is not None:
self.logger_manager.record(
scheduler_stats=outputs.scheduler_stats,
iteration_stats=iteration_stats,
mm_cache_stats=self.processor.stat_mm_cache(),
)
self.do_log_stats_with_interval()
with record_function_or_nullcontext("LLM_ENGINE STEP: RECORD_STATS"):
if self.logger_manager is not None and outputs.scheduler_stats is not None:
self.logger_manager.record(
scheduler_stats=outputs.scheduler_stats,
iteration_stats=iteration_stats,
mm_cache_stats=self.processor.stat_mm_cache(),
)
self.do_log_stats_with_interval()

@frank-wei frank-wei changed the title add scoping for better trace [Misc] Add more scoping for better trace Nov 8, 2025
@frank-wei frank-wei changed the title [Misc] Add more scoping for better trace [Misc] Add more scoping for improved trace Nov 8, 2025
frank-wei added a commit to frank-wei/vllm that referenced this pull request Nov 8, 2025
Summary:

Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Reviewed By: henryoier

Differential Revision: D86436375
frank-wei added a commit to frank-wei/vllm that referenced this pull request Nov 8, 2025
Summary:

Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Reviewed By: henryoier

Differential Revision: D86436375
@22quinn 22quinn requested a review from zhuohan123 November 8, 2025 00:26
frank-wei added a commit to frank-wei/vllm that referenced this pull request Nov 8, 2025
Summary:

Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Reviewed By: henryoier

Differential Revision: D86436375
frank-wei added a commit to frank-wei/vllm that referenced this pull request Nov 8, 2025
Summary:

Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Reviewed By: henryoier

Differential Revision: D86436375
@bangshengtang
Copy link
Collaborator

nit: can we try to unify the text format? we have all lower, all upper, first letter capitalized in three different layers

@frank-wei
Copy link
Contributor Author

frank-wei commented Nov 8, 2025

nit: can we try to unify the text format? we have all lower, all upper, first letter capitalized in three different layers

l prefer to use all lower case which saves the space visually. Will make corresponding changes.

frank-wei added a commit to frank-wei/vllm that referenced this pull request Nov 8, 2025
Summary:

Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Reviewed By: henryoier

Differential Revision: D86436375
Summary:

Add more scoping in the hotspot area which can greatly help us to the find the cycle heavy area

Reviewed By: henryoier

Differential Revision: D86436375

Signed-off-by: Wei Wei <wwei6@meta.com>
Signed-off-by: Wei Wei <wwei6@meta.com>
Signed-off-by: Wei Wei <wwei6@meta.com>
Signed-off-by: Wei Wei <wwei6@meta.com>
@zhuohan123 zhuohan123 enabled auto-merge (squash) November 10, 2025 18:46
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Nov 10, 2025
@zhuohan123 zhuohan123 merged commit bf6a3d0 into vllm-project:main Nov 10, 2025
49 checks passed
mm_cache_stats=self.processor.stat_mm_cache(),
)
self.do_log_stats_with_interval()
with record_function_or_nullcontext("llm_genine step: record_stats"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo here and above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in #28584

@njhill
Copy link
Member

njhill commented Nov 13, 2025

I'm not a fan of this TBH. I think having these all over the code makes it really messy and harder to read and maintain. I'd hope we could instrument in a less invasive way, restructuring into functions if/where it makes sense from a code readability/modularity pov, or at minimum being more conservative/selective as to where these are placed.

xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Nov 13, 2025
Signed-off-by: Wei Wei <wwei6@meta.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
@frank-wei
Copy link
Contributor Author

I'm not a fan of this TBH. I think having these all over the code makes it really messy and harder to read. I'd hope we could instrument in a less invasive way, restructuring into functions if/where it makes sense from a code readability/modularity pov, or at minimum being more conservative/selective as to where these are placed.

thanks @njhill , do you have any code pointer for a clean way? I can follow it.

@njhill
Copy link
Member

njhill commented Nov 15, 2025

@frank-wei FYI I'm reverting the ones in core.py as part of #28768.

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

Labels

ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants