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

#0: Use small vector instead of std::vector for shapes to optimize allocations #14281

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

sminakov-tt
Copy link
Contributor

@sminakov-tt sminakov-tt commented Oct 25, 2024

Ticket

Problem description

Currently we use std::vector to represent shapes, which causes a lot of allocation. However, all of the shapes are relatively small in practice, so instead we can use boost::small_vector, which pre-allocates a few elements on stack, avoiding allocations for the small vectors

What's changed

Introduced ttnn::SmallVector, a wrapper over boost::small_vector
Went through the whole codebase, and replaced std::vector, which was referring to a shape with either ttnn::SmallVector or std::span where appropriate

[🤓 PERF IMPACT INFO IN-PROGRESS]

Checklist

  • Post commit CI passes
  • Blackhole Post commit (if applicable)
  • Model regression CI testing passes (if applicable)
  • Device performance regression CI testing passes (if applicable)
  • New/Existing tests provide coverage for changes

@tt-aho
Copy link
Contributor

tt-aho commented Oct 25, 2024

Question on if we should push the SmallVector wrapper to tt_metal instead of ttnn?
I recently changed the internal of CoreRangeSet to be std::vector, but in most typical cases it would most likely be <=3 elements in most cases from my experience so might also be useful to swap to smallvector there as well.

@davorchap
Copy link
Collaborator

Question on if we should push the SmallVector wrapper to tt_metal instead of ttnn? I recently changed the internal of CoreRangeSet to be std::vector, but in most typical cases it would most likely be <=3 elements in most cases from my experience so might also be useful to swap to smallvector there as well.

I think this is a great idea

@sminakov-tt wdyt?

@sminakov-tt
Copy link
Contributor Author

@davorchap @tt-aho Yeah, I think we need small_vector everywhere :)
The issue is, that the amount of elements is embedded into the type, and the number is different depending on the use-case. For example in ttnn I picked 8, because historically we didn't allow shapes greater than 8, so the cases with more than 8 elements don't happen in practice. There are places where 8 would be too much, and places where 8 is too little. I went through all usages of std::vector and seen both happen.

The SmallVector wrapper over boost::small_vector can't really be pushed to tt_metal though. It has a python binding, allowing us to pass it to/from python. This binding must be specified in the same file as SmallVector itself, otherwise template resolution chaotically loses this overload and chaotically fails on method-per-method basis. And I don't think we should introduce python binding into tt_metal in any way.

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.