Skip to content

Copy ReadResult.value into bytes once - #302

Closed
sjperkins wants to merge 1 commit into
google:masterfrom
sjperkins:single-copy-cord-to-python
Closed

sjperkins wants to merge 1 commit into
google:masterfrom
sjperkins:single-copy-cord-to-python

Conversation

@sjperkins

Copy link
Copy Markdown
Contributor

Summary

ReadResult.value and KvStore.__getitem__ convert the absl::Cord through a temporary std::string. That copies the payload twice. This PR reuses BytesFromCord from serialization.cc, which copies the cord's chunks straight into a new bytes object.

 py::bytes CordToPython(const absl::Cord& value) {
-  return py::bytes(std::string(value));
+  auto obj = BytesFromCord(value);
+  if (!obj) throw py::error_already_set();
+  return py::reinterpret_steal<py::bytes>(obj.release());
 }

py::bytes has no constructor that takes over a std::string. Even if it had one, a PyBytesObject keeps its data inline, so the second copy can't be avoided by moving.

Evidence

Both builds use -c opt --copt=-O3, CPython 3.14 (with the GIL) on 12-core x86-64 Linux. A local HTTP server serves 15.5 MB objects.

master this PR
ReadResult.value, median per 15.5 MB object 10.2–11.9 ms 1.20–1.28 ms
page faults over 200 .value calls 1.59 M 78 K
http kvstore reads at 32 in flight, calling .value on each 1.03–1.27 GB/s 4.05–4.14 GB/s
the same reads, without .value 6.98 GB/s 6.87 GB/s

The speed-up is bigger than removing one copy alone would give. The temporary string and the bytes object were each mapped fresh on every call, costing about 7,900 page faults per 15.5 MB, and this PR avoids most of those.

python/tensorstore/tests/kvstore_test.py: 16 passed.

Merge Danger

Door: two-way

Blast Radius: KvStore.__getitem__ and ReadResult.value. Both still return the same bytes.

🤖 Generated with Claude Code

@google-cla

google-cla Bot commented Nov 2, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@sjperkins
sjperkins force-pushed the single-copy-cord-to-python branch from 282a277 to 7452a11 Compare November 2, 2026 13:42
CordToPython built `py::bytes(std::string(cord))`, which flattens the cord
into a temporary string and then copies that into the bytes object. Use
BytesFromCord instead, which copies the cord's chunks straight into a new
bytes object.
@sjperkins
sjperkins force-pushed the single-copy-cord-to-python branch from 7452a11 to 0490eab Compare November 2, 2026 13:54
@copybara-service copybara-service Bot closed this in 591c7ed Nov 3, 2026
@sjperkins
sjperkins deleted the single-copy-cord-to-python branch November 4, 2026 03:57
@sjperkins

Copy link
Copy Markdown
Contributor Author

Thanks for including this change. Could you please provide a pypi release with the above fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant