Skip to content

Use string_view in address methods #392

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

Open
wants to merge 3 commits into
base: branch-0.44
Choose a base branch
from
Open

Use string_view in address methods #392

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2edb816
Use `string_view` in address methods
jakirkham Mar 21, 2025
2aa957a
Reuse `length` assignment in allocation
jakirkham Mar 21, 2025
8277abd
Bump license header year range
jakirkham Mar 21, 2025
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
8 changes: 4 additions & 4 deletions cpp/include/ucxx/address.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
/**
* SPDX-FileCopyrightText: Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES.
* SPDX-FileCopyrightText: Copyright (c) 2022-2025, NVIDIA CORPORATION & AFFILIATES.
* SPDX-License-Identifier: BSD-3-Clause
*/
#pragma once

#include <memory>
#include <string>
#include <string_view>

#include <ucp/api/ucp.h>

Expand Down Expand Up @@ -75,7 +75,7 @@ class Address : public Component {
*
* @returns The `shared_ptr<ucxx::Address>` object.
*/
friend std::shared_ptr<Address> createAddressFromString(std::string addressString);
friend std::shared_ptr<Address> createAddressFromString(std::string_view addressString);

/**
* @brief Get the underlying `ucp_address_t*` handle.
Expand Down Expand Up @@ -112,7 +112,7 @@ class Address : public Component {
*
* @returns The underlying `ucp_address_t` handle.
*/
[[nodiscard]] std::string getString() const;
[[nodiscard]] std::string_view getString() const;
};

} // namespace ucxx
14 changes: 7 additions & 7 deletions cpp/src/address.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/**
* SPDX-FileCopyrightText: Copyright (c) 2022-2023, NVIDIA CORPORATION & AFFILIATES.
* SPDX-FileCopyrightText: Copyright (c) 2022-2025, NVIDIA CORPORATION & AFFILIATES.
* SPDX-License-Identifier: BSD-3-Clause
*/
#include <memory>
#include <string>
#include <string_view>

#include <ucxx/address.h>
#include <ucxx/utils/ucx.h>
Expand Down Expand Up @@ -38,21 +38,21 @@ std::shared_ptr<Address> createAddressFromWorker(std::shared_ptr<Worker> worker)
return std::shared_ptr<Address>(new Address(worker, address, length));
}

std::shared_ptr<Address> createAddressFromString(std::string addressString)
std::shared_ptr<Address> createAddressFromString(std::string_view addressString)
{
ucp_address_t* address = reinterpret_cast<ucp_address_t*>(new char[addressString.length()]);
size_t length = addressString.length();
memcpy(address, addressString.c_str(), length);
ucp_address_t* address = reinterpret_cast<ucp_address_t*>(new char[length]);
memcpy(address, addressString.data(), length);
return std::shared_ptr<Address>(new Address(nullptr, address, length));
}
Comment on lines -41 to 47
Copy link
Contributor

@wence- wence- Mar 31, 2025

Choose a reason for hiding this comment

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

This is fine.

Copy link
Member Author

@jakirkham jakirkham Apr 9, 2025

Choose a reason for hiding this comment

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

For additional context, this is used in one place in Cython where we already have something that could be std::string_view compliant (a Python bytes object). Currently in the construction we...

  1. Copy the bytes object into a std::string
  2. Copy the std::string to the createAddressFromString function call (no const std::string& currently)
  3. Allocate and copy the std::string to a char[]

ucxx/python/ucxx/ucxx/_lib/libucxx.pyx

Lines 407 to 416 in 4cd6354

@classmethod
def create_from_buffer(cls, bytes buf) -> UCXAddress:
cdef UCXAddress address = UCXAddress.__new__(UCXAddress)
cdef string address_str = string(<const char*>buf, len(buf))
with nogil:
address._address = createAddressFromString(address_str)
address._handle = address._address.get().getHandle()
address._length = address._address.get().getLength()
address._string = address._address.get().getString()

  1. Note there is a getString call at the end doing one more copy


ucp_address_t* Address::getHandle() const { return _handle; }

size_t Address::getLength() const { return _length; }

std::string Address::getString() const
std::string_view Address::getString() const
{
return std::string{reinterpret_cast<char*>(_handle), _length};
return std::string_view{reinterpret_cast<const char*>(_handle), _length};
}
Comment on lines -53 to 56
Copy link
Contributor

@wence- wence- Mar 31, 2025

Choose a reason for hiding this comment

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

NAK, this is horrible because now I have to remember to tie the lifetime of the returned object to the Address. And the docstring is now wrong.

I cannot believe this is worth it.

Copy link
Member

@pentschev pentschev Apr 8, 2025

Choose a reason for hiding this comment

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

This is a good point, we can make it the user's responsibility to ensure the lifetime, this is already the case for the UCX handles. I don't have a strong objection either way, I'm a little more inclined towards Lawrence's point simply because that would avoid the potential to dereference an invalid pointer, whereas if attempting to use the address (that has become invalid because the worker is not available anymore) would simply result in an error from UCX/exception from UCXX.

@wence- @jakirkham do you have strong opinions?

Copy link
Member Author

@jakirkham jakirkham Apr 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICT there are only two uses of this method. Both involve the creation of a Cython UCXAddress object

  1. From bytes as described above (currently 4 allocations & copies): Use string_view in address methods #392 (comment)
  2. From the Cython UCXWorker described below

In the Cython UCXWorker case, we take and store a copy of the std::shared_ptr<Address>. The Address in turn owns and keeps alive the ucp_address_t*/char* representation. Additionally we...

  1. Allocate and copy ucp_address_t* to std::string
  2. Copy this std::string into the Cython UCXAddress

ucxx/python/ucxx/ucxx/_lib/libucxx.pyx

Lines 395 to 405 in 4cd6354

@classmethod
def create_from_worker(cls, UCXWorker worker) -> UCXAddress:
cdef UCXAddress address = UCXAddress.__new__(UCXAddress)
with nogil:
address._address = worker._worker.get().getAddress()
address._handle = address._address.get().getHandle()
address._length = address._address.get().getLength()
address._string = address._address.get().getString()
return address

In both cases we are doing a fair bit of intermediate object creation and copying. The goal of this PR was to cutdown on that

Though would acknowledge that

  1. Many ways to achieve the same goal
  2. Some amount of creation and copying may be needed

That said, think it would be good if we could reduce the object creation and copying. Ideally deferring copying until the final object is constructed

Side note: std::string can be trivially constructed from a std::string_view


} // namespace ucxx
Loading