Use `string_view` in address methods #392

jakirkham · 2025-03-21T05:21:15Z

When setting or getting the underlying string data used for an Address object, avoid extra copying by using string_view. During initialization a copy is performed into the Address object's local stroage, which makes sense. Though the input to the function need not copy to string. It can just be viewed while the data is copied into the object's local memory. Further when converting the Address to a string, a view can be used to avoid a copy during the return. If the caller would like to persist the string longer than the lifetime of the Address object, they can copy it at that point. Though we can save them a copy before then. Especially if they will use the string while the Address object is present.

When setting or getting the underlying string data used for an `Address` object, avoid extra copying by using `string_view`. During initialization a copy is performed into the `Address` object's local stroage, which makes sense. Though the input to the function need not copy to `string`. It can just be viewed while the data is copied into the object's local memory. Further when converting the `Address` to a string, a view can be used to avoid a copy during the return. If the caller would like to persist the string longer than the lifetime of the `Address` object, they can copy it at that point. Though we can save them a copy before then. Especially if they will use the string while the `Address` object is present.

pentschev · 2025-03-21T08:25:00Z

Thanks for submitting this John! Unsurprisingly the tests failed:

ImportError: /opt/conda/conda-bld/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_pl/lib/python3.12/site-packages/ucxx/_lib/libucxx.cpython-312-x86_64-linux-gnu.so: undefined symbol: _ZN4ucxx23createAddressFromStringENSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE

That's because the Cython API has not been updated as well. Should we make all the necessary changes here, including bumping Cython to 3.1 and leave the PR ready for the time Cython 3.1 is out?

jakirkham · 2025-03-25T23:39:01Z

Ah sometimes the Cython API will let these things slide as it is usually just checking that something will work with the underlying C++ API (as opposed to enforcing it)

We could certainly do that

For now would it make sense to use const std::string&? That would save us some copies between these API calls

wence- · 2025-03-31T10:23:13Z

cpp/src/address.cpp


-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));
 }


This is fine.

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...

Copy the bytes object into a std::string

Copy the std::string to the createAddressFromString function call (no const std::string& currently)

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()

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

wence- · 2025-03-31T10:25:14Z

cpp/src/address.cpp


-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};
 }


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.

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?

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

From bytes as described above (currently 4 allocations & copies): Use string_view in address methods #392 (comment)

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...

Allocate and copy ucp_address_t* to std::string

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

Many ways to achieve the same goal

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

pentschev · 2025-04-08T14:27:56Z

For now would it make sense to use const std::string&? That would save us some copies between these API calls

Yes, using const std::string& immediately sounds appropriate to me

jakirkham requested a review from a team as a code owner March 21, 2025 05:21

jakirkham added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels Mar 21, 2025

jakirkham force-pushed the mk_add_sv branch from a293a9f to 2edb816 Compare March 21, 2025 05:29

jakirkham added 2 commits March 20, 2025 22:41

Reuse length assignment in allocation

2aa957a

Bump license header year range

8277abd

pentschev mentioned this pull request Mar 21, 2025

Remove UCXAddress.string in favor of __bytes__ #390

Merged

wence- requested changes Mar 31, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use `string_view` in address methods #392

Use `string_view` in address methods #392

jakirkham commented Mar 21, 2025

Uh oh!

pentschev commented Mar 21, 2025

Uh oh!

jakirkham commented Mar 25, 2025

Uh oh!

wence- Mar 31, 2025

Uh oh!

jakirkham Apr 9, 2025

Uh oh!

wence- Mar 31, 2025

Uh oh!

pentschev Apr 8, 2025

Uh oh!

jakirkham Apr 9, 2025 •

edited

Loading

Uh oh!

pentschev commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	@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()

	@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

Use string_view in address methods #392

Are you sure you want to change the base?

Use string_view in address methods #392

Conversation

jakirkham commented Mar 21, 2025

Uh oh!

pentschev commented Mar 21, 2025

Uh oh!

jakirkham commented Mar 25, 2025

Uh oh!

wence- Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

jakirkham Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

wence- Mar 31, 2025

Choose a reason for hiding this comment

Uh oh!

pentschev Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

jakirkham Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pentschev commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Use `string_view` in address methods #392

Use `string_view` in address methods #392

jakirkham Apr 9, 2025 •

edited

Loading