Skip to content

fix: keep host sources alive for async route-view copies (#1756) #1759

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.

Sign up for GitHub

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

Merged
rapids-bot merged 4 commits into from
Aug 21, 2026
Merged

fix: keep host sources alive for async route-view copies (#1756) #1759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
07a50d0
fix: keep host sources alive for async route-view copies (#1756)
ramakrishnap-nv Aug 21, 2026
ce25ffc
refactor: publish route views through a single path
ramakrishnap-nv Aug 21, 2026
9b8360d
review: publish route views by range, drop redundant static_assert
ramakrishnap-nv Aug 21, 2026
b0b31ce
review: make h_routes_view private
ramakrishnap-nv Aug 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 46 additions & 28 deletions cpp/src/routing/solution/solution.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,11 +93,10 @@ void solution_t<i_t, f_t, REQUEST>::add_route(route_t<i_t, f_t, REQUEST>&& route
route.n_nodes.set_value_async(n_nodes, sol_handle->get_stream());
route.route_id.set_value_async(route_id, sol_handle->get_stream());
sol_handle->sync_stream();
i_t route_slot = route_id_to_idx[route_id];
routes[route_slot] = std::move(route);
const auto route_view = routes[route_slot].view();
i_t route_slot = route_id_to_idx[route_id];
routes[route_slot] = std::move(route);
cuopt_assert(route_id < (int)routes_view.size(), "route id should be in range");
routes_view.set_element_async(route_id, route_view, sol_handle->get_stream());
set_route_views(route_id, route_id + 1);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
if (max_nodes_per_route < get_route(route_id).max_nodes_per_route()) {
resize_routes(raft::alignTo(get_route(route_id).max_nodes_per_route(), base_route_size));
}
Expand All @@ -122,6 +121,14 @@ void solution_t<i_t, f_t, REQUEST>::add_routes(
n_routes += added_routes;

check_and_allocate_routes(n_routes);
// Host sources of the async copies below must stay valid and unmodified until the
// sync_stream() after this loop, so per-iteration locals cannot be used. vehicle_id and
// route are references into new_routes and already outlive this function; the scalars
// are staged here, reserved up front so no reallocation can invalidate a pending copy.
std::vector<i_t> h_n_nodes;
std::vector<i_t> h_route_ids;
h_n_nodes.reserve(new_routes.size());
h_route_ids.reserve(new_routes.size());
for (const auto& [vehicle_id, route] : new_routes) {
// depot for the beginning and the end
i_t new_route_size = route.size() + 2;
Expand All @@ -131,26 +138,22 @@ void solution_t<i_t, f_t, REQUEST>::add_routes(
std::max(max_nodes_per_route, raft::alignTo(new_route_size, base_route_size));
resize_routes(new_route_size);
}
std::vector<NodeInfo<i_t>> node_info_h;
for (size_t x = 0; x < route.size(); ++x) {
// Dummy, will be overriden in set_nodes_data_of_route
node_info_h.emplace_back(route[x]);
}
// skip depot
// Values are overridden in set_nodes_data_of_route; copy straight out of new_routes,
// which outlives this function, rather than through a temporary. Skip the depot.
raft::copy(d_route.dimensions.requests.node_info.data() + 1,
node_info_h.data(),
node_info_h.size(),
route.data(),
route.size(),
sol_handle->get_stream());
const i_t n_nodes = route.size() + 1;
const i_t& n_nodes = h_n_nodes.emplace_back(route.size() + 1);
d_route.n_nodes.set_value_async(n_nodes, sol_handle->get_stream());
d_route.route_id.set_value_async(route_id, sol_handle->get_stream());
const i_t& stable_route_id = h_route_ids.emplace_back(route_id);
d_route.route_id.set_value_async(stable_route_id, sol_handle->get_stream());
d_route.vehicle_id.set_value_async(vehicle_id, sol_handle->get_stream());
i_t route_slot = route_id_to_idx[route_id];
const auto route_view = routes[route_slot].view();
cuopt_assert(route_id < (int)routes_view.size(), "route id should be in range");
routes_view.set_element_async(route_id, route_view, sol_handle->get_stream());
++route_id;
}
// Publish once, through the single path that owns the lifetime rule.
set_route_views(prev_route_size, n_routes);
set_nodes_data_of_new_routes(added_routes, prev_route_size);
sol_handle->sync_stream();
}
Expand Down Expand Up @@ -286,18 +289,32 @@ void solution_t<i_t, f_t, REQUEST>::set_routes_to_search()
}

template <typename i_t, typename f_t, request_t REQUEST>
void solution_t<i_t, f_t, REQUEST>::set_route_views()
void solution_t<i_t, f_t, REQUEST>::set_route_views(i_t start, i_t end)
{
raft::common::nvtx::range fun_scope("set_route_views");
if (routes.size() > routes_view.size()) {
// reserve with the max size
routes_view.resize(routes.size(), sol_handle->get_stream());
}

for (size_t i = 0; i < routes.size(); ++i) {
const auto route_view = get_route(i).view();
routes_view.set_element_async(i, route_view, sol_handle->get_stream());
if (end < 0) { end = (i_t)routes.size(); }
cuopt_assert(start >= 0 && end <= (i_t)routes.size(), "route view range out of bounds");
if (end <= start) { return; }

// Single point where route views are published to the device. The host source of an async
// copy must stay valid and unmodified until the stream is synchronized -- rmm's
// memcpy_async uses cudaMemcpySrcAccessOrderStream on CUDA 13, so the bytes are read when
// the copy runs, not when it is enqueued. Staging in a member buffer and synchronizing here
// keeps that rule in one place instead of at every call site.
h_routes_view.clear();
h_routes_view.reserve(end - start);
for (i_t i = start; i < end; ++i) {
h_routes_view.push_back(get_route(i).view());
}
raft::copy(routes_view.data() + start,
h_routes_view.data(),
h_routes_view.size(),
sol_handle->get_stream());
sol_handle->sync_stream();

@akifcorduk akifcorduk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am worried about this synchronization. I think we can remove that, I expect the call sites handle the synchronization pretty well. It was not synhronized before, it shouldn't be synchonized now.

@ramakrishnap-nv ramakrishnap-nv Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to push back on this one, or at least lay out why I think it is load-bearing.

The unsynchronized version is what caused #1756. Without a sync inside set_route_views, the host source has to outlive the copy, which is why the staging moved to a member buffer. That leaves exactly one hazard: a second publish overwriting the buffer while the first copy is still pending. It is reachable in add_route:

set_route_views(route_id, route_id + 1);            // copy A enqueued, reads h_routes_view
if (max_nodes_per_route < ...) resize_routes(...);  // republishes -> overwrites h_routes_view

With the sync removed, resize_routes can overwrite the staging before copy A executes. Copy A would then publish the post-resize view, i.e. a pointer to a buffer whose stream-ordered allocation has not run yet. Any kernel between the two dereferences it. Same failure class as the original bug, just narrower.

On "the call sites handle the synchronization pretty well" -- that was the assumption that did not hold. rmm sets srcAccessOrder = cudaMemcpySrcAccessOrderStream on CUDA 13 (rapidsai/rmm#2511), so the host bytes are read when the copy runs, not when it is enqueued. Before that change cudaMemcpyAsync staged small pageable copies during the API call, which is why the old code worked for years despite being UB by contract.

That said, your perf concern is fair and I do not want a blocking sync in a hot path either. The principled way to get both is to record a cudaEvent after the copy and wait on it only before reusing the staging buffer. In steady state the event is long complete so it costs nothing, and correctness stops depending on call-site discipline. Happy to implement that instead if you prefer.

Two mitigations already in place: resize_routes only republishes when something actually resized, and with the range argument each publish is now bounded, so this is not per-iteration of the solver. I have not profiled it though, and CodeRabbit asked for exactly that -- if you would rather I measure before we decide, I can.

@ramakrishnap-nv ramakrishnap-nv Aug 21, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@akifcorduk What is your suggestion on this #1759 (comment)

@akifcorduk akifcorduk Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

okay makes sense. we can keep it.

}

// init the rotues with the desired number of routes
Expand Down Expand Up @@ -374,14 +391,17 @@ void solution_t<i_t, f_t, REQUEST>::resize_routes(i_t new_size)
{
raft::common::nvtx::range fun_scope("resize_routes");

bool any_resized = false;
for (i_t i = 0; i < n_routes; ++i) {
auto& route_i = get_route(i);
if (route_i.max_nodes_per_route() < new_size) {
route_i.resize(new_size);
const auto route_view = get_route(i).view();
routes_view.set_element_async(i, route_view, sol_handle->get_stream());
any_resized = true;
}
}
// A resize reallocates the route buffers, so the device-side views are stale. Republish
// through set_route_views rather than copying each changed view from a local here.
if (any_resized) { set_route_views(); }
max_nodes_per_route = std::max(max_nodes_per_route, new_size);
}

Expand Down Expand Up @@ -512,10 +532,8 @@ void solution_t<i_t, f_t, REQUEST>::copy_device_solution(solution_t<i_t, f_t, RE
if (!src_sol.n_routes) { return; }
check_and_allocate_routes(src_sol.n_routes);

for (i_t i = n_routes; i < src_sol.n_routes; ++i) {
const auto route_view = get_route(i).view();
routes_view.set_element_async(i, route_view, sol_handle->get_stream());
}
// copy_routes below reads these entries, so they must be published before it launches.
set_route_views(n_routes, src_sol.n_routes);

n_routes = src_sol.n_routes;

Expand Down
10 changes: 8 additions & 2 deletions cpp/src/routing/solution/solution.cuh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* clang-format off */
/*
* SPDX-FileCopyrightText: Copyright (c) 2022-2025, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-FileCopyrightText: Copyright (c) 2022-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*/
/* clang-format on */
Expand Down Expand Up @@ -494,7 +494,8 @@ class solution_t {
size_t get_temp_route_shared_size(i_t added_size = 0) const;
void compute_initial_data(bool check_feasibility = true);
void random_init_routes();
void set_route_views();
// Publishes routes[start, end) to the device. end < 0 means routes.size().
void set_route_views(i_t start = 0, i_t end = -1);
void expand_route(i_t route_id);
void resize_route(i_t route_id, i_t new_route_size);
void clear_routes(std::vector<i_t> vehicle_ids);
Expand Down Expand Up @@ -650,6 +651,11 @@ class solution_t {
// we shouldn't access this directly as route ids map differently
std::vector<route_t<i_t, f_t, REQUEST>> routes;

// Host staging for routes_view, internal to set_route_views(). That function synchronizes
// before returning, so no copy is ever left pending against this buffer and it is safe to
// reuse across calls. Kept as a member so the paths that publish views do not allocate.
std::vector<typename route_t<i_t, f_t, REQUEST>::view_t> h_routes_view;

public:
static constexpr i_t fragment_step = 1;

Expand Down
8 changes: 0 additions & 8 deletions python/cuopt/cuopt/tests/routing/test_batch_solve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@
# SPDX-License-Identifier: Apache-2.0

import cudf
import cupy
import numpy as np
import pytest

from cuopt import routing

Expand All @@ -18,12 +16,6 @@ def create_tsp_cost_matrix(n_locations):
return cudf.DataFrame(cost_matrix)


@pytest.mark.skipif(
13030 <= cupy.cuda.get_local_runtime_version() < 13040,
reason="block_copy destination overflow on CUDA 13.3, see "
"https://github.com/NVIDIA/cuopt/issues/1756. The device-side assert "
"aborts the process, so xfail cannot catch it.",
)
def test_batch_solve_varying_sizes():
"""Test batch solving TSPs of varying sizes."""
tsp_sizes = [
Expand Down