Skip to content

Commit 2e4bd22

Browse files
committed
Address review: wait for the device around every call instead of per-result flags
A review of the merged pybindings change found one correctness bug and several rough edges. The main bug: a CUDA result borrowed from a method shared the method's output memory, but whether another library might be writing that memory was tracked with a per-result flag that a borrowed result started fresh. So a borrowed result written by another library and passed back in could be read before that write landed. This replaces the flags with one rule: wait for the device once before and once after each call, and always before handing CUDA memory out through __dlpack__ or to_host(). The extra wait measured about a microsecond per call. Also: - A result of these bindings fed back in now takes the DLPack path, which keeps its real dtype and strides, so a bfloat16 or channels-last result is no longer read in the wrong order. - A failed device allocation no longer disables the no-copy output return for the rest of a method's life: the new buffer is allocated into a local first. - A clone_outputs=False read is a separate borrowed view, not the cached handed-over object from a default read. - Errors raised while reading an input keep their exact Python type, by adding a note instead of rebuilding the exception. - Every input error names the input and method, including the device check and the unsupported-type error.
1 parent c1d7efb commit 2e4bd22

5 files changed

Lines changed: 140 additions & 153 deletions

File tree

‎extension/pybindings/README.md‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,12 @@ runtime or input storage and later execution may overwrite that storage.
4747
A CUDA output that fills memory the bindings allocated for it is returned
4848
without a copy when `clone_outputs=True`. The result keeps that memory, so a
4949
method writes its next execution into a second buffer, and the two take turns;
50-
a module allocates new memory each call. Reading the outputs again before the
51-
next execution returns the same object, so an in-place change to one read
52-
shows in the other. In torch output mode such a result is a view of memory
53-
torch does not own, so it cannot grow with `resize_()`.
50+
a module allocates new memory each call. Within one execution, reading the
51+
outputs again with `clone_outputs=True` returns the same object, so an in-place
52+
change to one read shows in the other; a `clone_outputs=False` read is a
53+
separate borrowed view. In torch output mode such a result is a view of memory
54+
torch does not own, so it cannot grow with `resize_()` and cannot be shared
55+
through `torch.multiprocessing`. Call `.clone()` for a normal torch tensor.
5456

5557
`dtype_name` names a result's dtype, including dtypes NumPy lacks, such as
5658
`"bfloat16"`.
@@ -103,11 +105,13 @@ CUDA DLPack inputs and `ExecuTorchResult` outputs require CUDA-enabled bindings.
103105
Torch CUDA inputs and outputs synchronize through Python Torch, including in
104106
CPU builds of the bindings. Execution is synchronous at the Python boundary:
105107
a call returns after its device work has finished, so its outputs are ready on
106-
any stream. Each call waits for the device once, rather than once per tensor.
107-
Before running, it also waits once if an input may still be written on another
108-
library's stream: a torch CUDA tensor, a DLPack input from another library, or
109-
an `ExecuTorchResult` whose memory was handed out through `__dlpack__`. The
110-
synchronization has no C++ ATen dependency.
108+
any stream. Each call waits for the device once after it runs, rather than once
109+
per output. Before running, it waits once per device for any CUDA input that
110+
another library may still be writing: a torch CUDA tensor, a CUDA DLPack input,
111+
or a result of these bindings passed back in, since that result can share memory
112+
a caller handed to another library. Reading a CUDA result through `__dlpack__`
113+
or `to_host()` waits once as well. The synchronization has no C++ ATen
114+
dependency.
111115

112116
For bfloat16 inputs, raw byte buffers use the shape declared by the exported
113117
method. Bfloat16 results expose flat bytes through the buffer protocol because

‎extension/pybindings/pybindings.cpp‎

Lines changed: 72 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -313,11 +313,7 @@ class BufferTensor final {
313313
info_(buffer_.request()) {
314314
check_rank();
315315
const auto input_meta = method_meta.input_tensor_meta(input_index);
316-
THROW_IF_ERROR(
317-
input_meta.error(),
318-
"Input %zu of method '%s' is not a tensor input",
319-
input_index,
320-
method_meta.name());
316+
THROW_IF_ERROR(input_meta.error(), "is not a tensor input");
321317
const bool raw_bfloat16 =
322318
input_meta->scalar_type() == executorch::aten::ScalarType::BFloat16 &&
323319
info_.ndim == 1 &&
@@ -329,8 +325,8 @@ class BufferTensor final {
329325
}
330326
if (static_cast<size_t>(info_.size) != input_meta->nbytes()) {
331327
throw py::value_error(
332-
"Raw bfloat16 input " + std::to_string(input_index) + " has " +
333-
std::to_string(info_.size) + " bytes, but the method expects " +
328+
"raw bfloat16 input has " + std::to_string(info_.size) +
329+
" bytes, but the method expects " +
334330
std::to_string(input_meta->nbytes()));
335331
}
336332
scalar_type_ = executorch::aten::ScalarType::BFloat16;
@@ -658,24 +654,29 @@ bool is_torch_tensor(const py::handle& value) {
658654
return result == 1;
659655
}
660656

661-
// Runs `read`, which builds one input from a Python object, and starts the
662-
// message of any error it raises with the input and its method, which the
663-
// readers cannot see. The error keeps its Python type.
657+
// Runs `read`, which builds one input from a Python object, and names the
658+
// input and its method in any error it raises, which the readers cannot see.
664659
template <typename Read>
665660
auto read_input(const MethodMeta& method_meta, size_t index, Read&& read) {
666-
const auto named = [&](const char* message) {
667-
return "Input " + std::to_string(index) + " of method '" +
668-
method_meta.name() + "': " + message;
661+
const std::string prefix = "Input " + std::to_string(index) + " of method '" +
662+
method_meta.name() + "'";
663+
const auto named = [&prefix](const char* message) {
664+
return prefix + ": " + message;
669665
};
670666
try {
671667
return read();
672668
} catch (py::error_already_set& error) {
673-
// Chained to the original, so its traceback stays available.
674-
const auto type = error.type();
675-
const auto message =
676-
named(py::str(error.value()).cast<std::string>().c_str());
677-
py::raise_from(error, type.ptr(), message.c_str());
678-
throw py::error_already_set();
669+
// Keep the exact exception and its attributes, adding the input name as a
670+
// note (Python 3.11+). Rebuilding it as type(message) would fail for any
671+
// exception whose constructor is not a single string.
672+
if (error.value()) {
673+
try {
674+
error.value().attr("add_note")(prefix);
675+
} catch (...) {
676+
// add_note is best effort; never hide the original error.
677+
}
678+
}
679+
throw;
679680
} catch (const py::buffer_error& error) {
680681
throw py::buffer_error(named(error.what()));
681682
} catch (const py::value_error& error) {
@@ -684,9 +685,9 @@ auto read_input(const MethodMeta& method_meta, size_t index, Read&& read) {
684685
throw py::type_error(named(error.what()));
685686
} catch (const std::invalid_argument& error) {
686687
throw py::value_error(named(error.what()));
687-
} catch (const std::exception& error) {
688-
throw std::runtime_error(named(error.what()));
689688
}
689+
// Any other C++ exception passes through so pybind11 maps it to the right
690+
// Python type (std::bad_alloc to MemoryError, and so on).
690691
}
691692

692693
void validate_tensor_input(
@@ -872,7 +873,8 @@ class TorchTensorView final {
872873
: "cuda:" + std::to_string(device.index());
873874
};
874875
throw py::value_error(
875-
"Input " + std::to_string(index) + " is on device " + name(device_) +
876+
"Input " + std::to_string(index) + " of method '" +
877+
method.method_meta().name() + "' is on device " + name(device_) +
876878
", but the method expects " + name(expected));
877879
}
878880
}
@@ -995,16 +997,11 @@ class DLPackTensorView final {
995997
advertised_device_(
996998
owner_.attr("__dlpack_device__")().cast<std::pair<int, int>>()) {
997999
validate_advertised_device();
998-
// A result of these bindings is written either by an execution, which
999-
// finished before it returned, or by a copy on the legacy default stream,
1000-
// which the delegates' streams wait for. So it needs no wait unless it
1001-
// was handed to another library. Anything else may still be written on
1002-
// its producer's streams until the call waits for it.
1000+
// A result of these bindings exposes its memory directly, since the call
1001+
// that made it already waited for the device. Any other producer may
1002+
// still be writing on its own stream, so the call waits before it runs.
10031003
if (py::isinstance<PyExecuTorchResult>(owner_)) {
1004-
own_result_ = owner_.cast<std::shared_ptr<PyExecuTorchResult>>();
1005-
}
1006-
if (own_result_) {
1007-
capsule_ = own_result_->capsule();
1004+
capsule_ = owner_.cast<std::shared_ptr<PyExecuTorchResult>>()->capsule();
10081005
} else {
10091006
capsule_ = advertised_device_.first == dlpack::CUDA
10101007
? cuda_dlpack_capsule(owner_, advertised_device_.second)
@@ -1061,14 +1058,6 @@ class DLPackTensorView final {
10611058
return nbytes_;
10621059
}
10631060

1064-
// Whether the producer may still be writing the tensor on its own streams.
1065-
// Asked when the call runs, since a result set as an input can be handed to
1066-
// another library before that.
1067-
bool may_have_pending_writes() const {
1068-
return advertised_device_.first == dlpack::CUDA &&
1069-
!(own_result_ && !own_result_->handed_out());
1070-
}
1071-
10721061
// Describes the tensor in the exported dimension order. A CPU tensor dense
10731062
// in another order is copied into that order, since reading it with the
10741063
// exported strides would permute its values. A device tensor would need a
@@ -1103,8 +1092,9 @@ class DLPackTensorView final {
11031092
if (nbytes_ != 0 &&
11041093
device_ != method.get_input(index).toTensor().device()) {
11051094
throw py::value_error(
1106-
"DLPack input " + std::to_string(index) +
1107-
" device does not match the method input device");
1095+
"Input " + std::to_string(index) + " of method '" +
1096+
method.method_meta().name() +
1097+
"': DLPack input device does not match the method input device");
11081098
}
11091099
}
11101100

@@ -1298,7 +1288,6 @@ class DLPackTensorView final {
12981288

12991289
py::object owner_;
13001290
std::pair<int, int> advertised_device_;
1301-
std::shared_ptr<PyExecuTorchResult> own_result_;
13021291
size_t nbytes_ = 0;
13031292
py::object capsule_;
13041293
dlpack::ManagedTensor* managed_ = nullptr;
@@ -1496,6 +1485,13 @@ bool prefer_dlpack_input(const py::handle& value) {
14961485
if (!PyObject_CheckBuffer(value.ptr())) {
14971486
return true;
14981487
}
1488+
// A result of these bindings always takes the DLPack path, so its real
1489+
// dtype and strides come through. Its buffer export is flat bytes, which the
1490+
// buffer path would read with the method's layout and scramble a result in
1491+
// another dim order, bfloat16 included.
1492+
if (py::isinstance<PyExecuTorchResult>(value)) {
1493+
return true;
1494+
}
14991495
// NumPy and other host buffers keep the buffer path, including its
15001496
// read-only copy behavior. CUDA results expose only their DLPack storage.
15011497
if (py::hasattr(value, "__dlpack_device__")) {
@@ -1566,9 +1562,10 @@ void synchronize_device_outputs(const Method& method) {
15661562
}
15671563

15681564
// Device inputs from another library may still be written on that library's
1569-
// streams, torch tensors included. One wait per call covers every such input,
1570-
// instead of one per input. An ExecuTorchResult these bindings returned needs
1571-
// no wait unless its memory was handed to another library since.
1565+
// streams, torch tensors and DLPack producers alike. One wait per device
1566+
// covers every such input. A result these bindings returned is included too:
1567+
// it can share memory a caller gave to another library, and a borrowed result
1568+
// cannot tell, so the call waits for its device rather than trusting a flag.
15721569
void synchronize_device_inputs(
15731570
const std::vector<std::shared_ptr<DLPackTensorView>>& dlpack_inputs,
15741571
const std::vector<std::shared_ptr<TorchTensorView>>& torch_inputs) {
@@ -1581,7 +1578,8 @@ void synchronize_device_inputs(
15811578
}
15821579
};
15831580
for (const auto& input : dlpack_inputs) {
1584-
if (input->nbytes() != 0 && input->may_have_pending_writes()) {
1581+
if (input->nbytes() != 0 &&
1582+
input->device().type() == executorch::aten::DeviceType::CUDA) {
15851583
add(input->device());
15861584
}
15871585
}
@@ -2558,7 +2556,8 @@ struct PyModule final {
25582556
#endif
25592557
} else {
25602558
throw std::runtime_error(
2561-
"Unsupported python type " + type_str +
2559+
"Input " + std::to_string(i) + " of method '" + method_meta.name() +
2560+
"': unsupported python type " + type_str +
25622561
". Ensure that inputs are passed as a flat list of tensors.");
25632562
}
25642563
}
@@ -3388,7 +3387,9 @@ struct PyMethod final {
33883387
#endif
33893388
} else {
33903389
throw std::runtime_error(
3391-
"Unsupported python type " + type_str +
3390+
"Input " + std::to_string(i) + " of method '" +
3391+
storage_->method->method_meta().name() +
3392+
"': unsupported python type " + type_str +
33923393
". Ensure that inputs are passed as a flat list of tensors.");
33933394
}
33943395
}
@@ -3598,21 +3599,13 @@ struct PyMethod final {
35983599
std::vector<py::object> handed_outputs_;
35993600
// Per device output buffer, alive while such a result still uses it.
36003601
std::vector<std::weak_ptr<void>> handed_buffers_;
3601-
// Per device output buffer, whether a result was handed it since the last
3602-
// execution.
3603-
std::vector<bool> handed_slots_;
36043602
// Per device output, the buffer a still-alive earlier result uses, kept so
36053603
// it can be reused once that result is dropped.
36063604
struct SpareDeviceOutput final {
36073605
std::shared_ptr<DeviceMemoryBuffer> buffer;
36083606
std::weak_ptr<void> handed;
3609-
std::shared_ptr<bool> handed_out;
36103607
};
36113608
std::vector<SpareDeviceOutput> spare_device_outputs_;
3612-
// Per device output buffer, whether another library may have read it, so
3613-
// reusing it must wait for that library's queued work. Torch outputs always
3614-
// may; an ExecuTorchResult only once exported through DLPack.
3615-
std::vector<std::shared_ptr<bool>> handed_out_;
36163609
#ifndef USE_ATEN_LIB
36173610

36183611
struct BorrowedOutputOwner final {
@@ -3690,10 +3683,7 @@ struct PyMethod final {
36903683
std::make_shared<std::shared_ptr<DeviceMemoryBuffer>>(buffer);
36913684
const std::shared_ptr<void> memory(holder, buffer->data());
36923685
py::object result;
3693-
const auto torch_output =
3694-
portable_tensor_output() == PortableTensorOutput::Torch;
3695-
handed_out_[*slot] = std::make_shared<bool>(torch_output);
3696-
if (torch_output) {
3686+
if (portable_tensor_output() == PortableTensorOutput::Torch) {
36973687
auto* retained = new std::shared_ptr<void>(memory);
36983688
result = portable_tensor_result(
36993689
tensor,
@@ -3702,11 +3692,9 @@ struct PyMethod final {
37023692
delete static_cast<std::shared_ptr<void>*>(pointer);
37033693
}));
37043694
} else {
3705-
result = py::cast(
3706-
PyExecuTorchResult::hand_over(tensor, memory, handed_out_[*slot]));
3695+
result = py::cast(PyExecuTorchResult::hand_over(tensor, memory));
37073696
}
37083697
handed_buffers_[*slot] = holder;
3709-
handed_slots_[*slot] = true;
37103698
handed_outputs_.resize(storage_->method->outputs_size());
37113699
handed_outputs_[index] = result;
37123700
return result;
@@ -3853,59 +3841,38 @@ struct PyMethod final {
38533841
storage_->output_storages = std::move(output_storages);
38543842
storage_->device_output_storages = std::move(device_output_storages);
38553843
handed_buffers_.assign(num_outputs, std::weak_ptr<void>());
3856-
handed_slots_.assign(num_outputs, false);
38573844
spare_device_outputs_.assign(num_outputs, SpareDeviceOutput());
3858-
handed_out_.assign(num_outputs, nullptr);
38593845
}
38603846

3861-
// A handed-over result still uses its buffer, so the method writes into
3862-
// its spare instead, and the two take turns. Borrowed results keep their
3863-
// buffer alive themselves, as they always could see the next execution's
3864-
// values.
3847+
// A handed-over result still uses its buffer, so the method writes into its
3848+
// spare instead, and the two take turns. Borrowed results keep their buffer
3849+
// alive themselves, as they always could see the next execution's values.
38653850
void replace_held_device_outputs() {
38663851
#ifndef USE_ATEN_LIB
38673852
for (size_t i = 0; i < storage_->device_output_storages.size(); ++i) {
38683853
auto& buffer = storage_->device_output_storages[i];
3869-
if (!buffer || !handed_slots_[i]) {
3854+
if (!buffer || handed_buffers_[i].expired()) {
38703855
continue;
38713856
}
3857+
// The buffer this slot points at is still held by a live result, so the
3858+
// next run must not write into it. Take the spare if a dropped result
3859+
// freed it, otherwise allocate a fresh one. Allocate into a local first,
3860+
// so a failed allocation leaves the slot and the spare untouched.
38723861
const auto device = storage_->method->get_output(i).toTensor().device();
3873-
if (!handed_buffers_[i].expired()) {
3874-
// In a loop such as `out = method(x)[0]` the previous result is still
3875-
// alive here, so its buffer and this one take turns instead of a new
3876-
// allocation each call.
3877-
auto& spare = spare_device_outputs_[i];
3878-
if (spare.buffer && spare.handed.expired()) {
3879-
std::swap(buffer, spare.buffer);
3880-
std::swap(handed_buffers_[i], spare.handed);
3881-
std::swap(handed_out_[i], spare.handed_out);
3882-
wait_for_other_readers(i, device);
3883-
} else {
3884-
spare = {buffer, handed_buffers_[i], handed_out_[i]};
3885-
buffer = allocate_device_output(i, buffer->size(), device);
3886-
}
3862+
auto& spare = spare_device_outputs_[i];
3863+
if (spare.buffer && spare.handed.expired()) {
3864+
std::swap(buffer, spare.buffer);
3865+
std::swap(handed_buffers_[i], spare.handed);
38873866
} else {
3888-
wait_for_other_readers(i, device);
3867+
auto fresh = allocate_device_output(i, buffer->size(), device);
3868+
spare = {std::move(buffer), handed_buffers_[i]};
3869+
buffer = std::move(fresh);
3870+
handed_buffers_[i].reset();
38893871
}
3890-
handed_buffers_[i].reset();
3891-
handed_out_[i].reset();
3892-
handed_slots_[i] = false;
38933872
}
38943873
#endif
38953874
}
38963875

3897-
#ifndef USE_ATEN_LIB
3898-
// A dropped result's memory may still be read by work another library
3899-
// queued on its own streams. Freeing it would have waited for that work, so
3900-
// reusing it waits too. Work these bindings queued has already finished,
3901-
// since every execution waits for its device before it returns.
3902-
void wait_for_other_readers(size_t index, executorch::aten::Device device) {
3903-
if (handed_out_[index] && *handed_out_[index]) {
3904-
synchronize_device(device.index());
3905-
}
3906-
}
3907-
#endif
3908-
39093876
py::list get_outputs_as_py_list(
39103877
const std::vector<EValue>& outputs,
39113878
bool clone_outputs = true) {
@@ -3933,7 +3900,11 @@ struct PyMethod final {
39333900
list[i] = py::cast(v.toTensor());
39343901
}
39353902
#else
3936-
if (i < handed_outputs_.size() && handed_outputs_[i]) {
3903+
// Within one execution, repeated default reads return the same handed
3904+
// over object, so a caller sees stable identity and the buffer is not
3905+
// handed over twice. A clone_outputs=False read is a separate borrowed
3906+
// view and never returns that cached object.
3907+
if (clone_outputs && i < handed_outputs_.size() && handed_outputs_[i]) {
39373908
list[i] = handed_outputs_[i];
39383909
continue;
39393910
}

0 commit comments

Comments
 (0)