Skip to content

Clean up code handling both std::vector and SmallVector#2493

Merged
zcbenz merged 1 commit intoml-explore:mainfrom
zcbenz:is-vector
Aug 16, 2025
Merged

Clean up code handling both std::vector and SmallVector#2493
zcbenz merged 1 commit intoml-explore:mainfrom
zcbenz:is-vector

Conversation

@zcbenz
Copy link
Copy Markdown
Collaborator

@zcbenz zcbenz commented Aug 14, 2025

Using SmallVector does not always have performance gain over std::vector in all cases so we are having code handling both std::vector and SmallVector, this PR adds a is_vector type trait to make the code easier to maintain.

@awni
Copy link
Copy Markdown
Member

awni commented Aug 15, 2025

Using SmallVector does not always have performance gain over std::vector in all cases so we are having code handling

Can you elaborate on that? Where do we not get perf gain / where are we using std::vector still for shapes and dtypes?

Copy link
Copy Markdown
Member

@awni awni left a comment

Choose a reason for hiding this comment

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

Looks good. I'm slightly confused as to the purpose .. but feel free to merge the cleanup as it seems useful regardless.

@zcbenz
Copy link
Copy Markdown
Collaborator Author

zcbenz commented Aug 15, 2025

Can you elaborate on that? Where do we not get perf gain / where are we using std::vector still for shapes and dtypes?

I made some experiments replacing more std::vector with SmallVector:

  • replacing op args like std::vector<int> axes to SmallVector.
  • replacing all std::vector<array> with SmallVector.

and none gave signs of performance gain or regression. Intuitively I think it is because when a std::vector is only allocated once and almost never copied, the cost of heap allocation is negligible. For shape/strides the story is different because they are frequently copied and resized in performance critical code paths.

Currently all shape/strides are using SmallVector, but some code needs to handle both std::vector and SmallVector because some kernel args like axes and padding are still stored in std::vector.

@awni
Copy link
Copy Markdown
Member

awni commented Aug 15, 2025

Makes sense, thanks for the explanation!

@zcbenz zcbenz merged commit 37b440f into ml-explore:main Aug 16, 2025
7 checks passed
@zcbenz zcbenz deleted the is-vector branch August 16, 2025 00:01
faisalmemon pushed a commit to faisalmemon/mlx that referenced this pull request Oct 30, 2025
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