Skip to content

Remove the per-op completion handler in gpu::eval - #4048

Open
kernelpool wants to merge 1 commit into
ml-explore:mainfrom
kernelpool:metal-batch-completion-handlers
Open

Remove the per-op completion handler in gpu::eval#4048
kernelpool wants to merge 1 commit into
ml-explore:mainfrom
kernelpool:metal-batch-completion-handlers

Conversation

@kernelpool

Copy link
Copy Markdown
Contributor

Proposed changes

gpu::eval registers an empty addCompletedHandler and builds an unordered_set of shared_ptrs for every operation, in order to keep that op's inputs alive until the command buffer completes. CommandEncoder::commit() already installs one handler per command buffer, so the per-op handlers are redundant and the buffers can go there instead. On an M3 Ultra that takes mlx-community/Qwen3.5-0.8B-4bit decode from 280 to 367 tok/s. Bandwidth-bound models are unchanged.

Before

mlx_lm.benchmark --model mlx-community/Qwen3.5-0.8B-4bit -p 2048 -g 128 -n 3
Running warmup..
Timing with prompt_tokens=2048, generation_tokens=128, batch_size=1.
Trial 1:  prompt_tps=13108.539, generation_tps=284.374, peak_memory=1.886, total_time=0.744
Trial 2:  prompt_tps=13404.757, generation_tps=278.961, peak_memory=1.886, total_time=0.749
Trial 3:  prompt_tps=13181.042, generation_tps=277.251, peak_memory=1.887, total_time=0.753
Averages: prompt_tps=13231.446, generation_tps=280.195, peak_memory=1.886

After

mlx_lm.benchmark --model mlx-community/Qwen3.5-0.8B-4bit -p 2048 -g 128 -n 3
Running warmup..
Timing with prompt_tokens=2048, generation_tokens=128, batch_size=1.
Trial 1:  prompt_tps=13288.070, generation_tps=365.077, peak_memory=1.886, total_time=0.637
Trial 2:  prompt_tps=13271.215, generation_tps=361.761, peak_memory=1.886, total_time=0.650
Trial 3:  prompt_tps=13402.103, generation_tps=376.355, peak_memory=1.887, total_time=0.629
Averages: prompt_tps=13320.463, generation_tps=367.731, peak_memory=1.886

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

Comment on lines +140 to +141
std::vector<std::shared_ptr<array::Data>> retained_buffers_;
std::unordered_set<const array::Data*> retained_ptrs_;

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 think you can just store buffers in the set.

Suggested change
std::vector<std::shared_ptr<array::Data>> retained_buffers_;
std::unordered_set<const array::Data*> retained_ptrs_;
std::unordered_set<std::shared_ptr<array::Data>> retained_buffers_;

});
} else {
command_buffer->addCompletedHandler(
[buffers = std::move(buffers)](MTL::CommandBuffer* cbuf) {});

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.

The downside of this approach is that buffers are now retained until the command encoder is committed, which I think could increase peak memory a lot? Can you check the memory usage in a few popular models?

I support reducing the completion handler to have lower overhead but we probably need to add some heuristics.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants