#14366: Allow creating pre-allocated buffer with an address. Minor perf improvements #14394
Conversation
sminakov-tt
requested review from
abhullar-tt,
pgkeller,
aliuTT,
tt-aho,
tt-dma,
tt-asaigal,
davorchap and
ubcheema
as code owners
| @@ -144,6 +144,8 @@ struct BufferPageMapping { | |||
| inline namespace v0 { | |||
|
|
|||
| class Buffer final { | |||
| struct Private { explicit Private() = default; }; | |||
There was a problem hiding this comment.
The Private structure is used to limit access to the Buffer constructor, which can't be truly private, because of std::make_shared. This is the way recommended by CppReference documentation: https://en.cppreference.com/w/cpp/memory/enable_shared_from_this (see Best example)
| @@ -240,16 +244,38 @@ std::shared_ptr<Buffer> Buffer::create( | |||
| buffer->address_ = detail::AllocateBuffer(buffer.get()); | |||
|
|
|||
| std::unique_lock lock(buffer->allocation_mutex_); | |||
| buffer->allocation_status_.store(AllocationStatus::ALLOCATED, std::memory_order::relaxed); | |||
| buffer->allocation_status_.store(AllocationStatus::ALLOCATED, std::memory_order::release); | |||
There was a problem hiding this comment.
Does this really need to be made release? It seems like we're already protected by a lock.
There was a problem hiding this comment.
Yes. In Buffer::address() the first check is allocation_status_.load(std::memory_order::acquire) != AllocationStatus::ALLOCATION_REQUESTED performed without a lock. This is done this way for performance optimization reasons. The only two places who capture a lock areBuffer::create when allocating and Buffer::address when waiting on an allocation using condition variable.
sminakov-tt
force-pushed
the
sminakov/buffer-address
branch
from
bbddcde to
ed11135
Compare
| @@ -289,6 +324,10 @@ bool Buffer::is_allocated() const { | |||
| } | |||
|
|
|||
| uint32_t Buffer::address() const { | |||
| if (allocation_status_.load(std::memory_order::acquire) != AllocationStatus::ALLOCATION_REQUESTED) { | |||
| return address_; | |||
There was a problem hiding this comment.
Optimization for the happy case, no waiting or other checks if buffer is already allocated.
…s. Minor perf improvements (tenstorrent#14394) * tenstorrent#14366: Allow creating pre-allocated buffer with an address. Minor performance improvements * #0: Cleanup * #0: Updated methods documentation * #0: Handle allocation failure * #0: Scheduling fix
Ticket
#14366
Problem description
During the recent Buffer refactoring,
Buffer::set_addressmethod was removed, which broke the clients who want to create a Buffer with an already allocated address.As mentioned in the ticket, a call to
can_use_passthrough_scheduling()inBuffer::address()is slower than expected.What's changed
Added another overload to
Buffer::createandCreateBuffer, which allows to specify the address.Stored a flag to indicate whether a Buffer owns the data it points to or not.
Use
std::make_sharedfor non-owning use case to optimize performance.Replaced the call to
can_use_passthrough_scheduling()inBuffer::address()with atomics to improve performanceChecklist