Add zero-copy serialization APIs. #357

teo-tsirpanis · 2024-11-26T17:05:35Z

This PR updates the Buffer and BufferList types to implement the io.WriterTo interface, by writing the data directly to an io.Writer without an intermediate copy to Go-backed memory. For Buffer we do that with the Unsafe.Slice function (it is safe as long as the Writer follows the contract and does not retain the slice after the call to Write), and for BufferList we call WriteTo for each buffer.

In order to take advantage of this, new serialization APIs were added that return a Buffer instead of a byte[] (and unavoidably had to make a copy to Go-managed memory). The old serialization APIs, as well as the Buffer.Serialize() and BufferList.Flatten() functions were deprecated.

shaunrd0

Left some small comments but LGTM, nice work!

shaunrd0 · 2024-11-26T19:00:40Z

buffer.go

+		// Keep the buffer alive during the write, to prevent the GC from
+		// collecting the memory while it's being used.
+		n, err := w.Write(unsafe.Slice((*byte)(cbuffer), writeSize))
+		runtime.KeepAlive(b)


I think since b is the object that implements this method the keepalive isn't needed

Removed. I was being extra cautious, but if we call b.WriteTo(w), it becomes clear that b is being kept somewhere reachable.

shaunrd0 · 2024-11-26T19:07:18Z

serialize.go

+	return &buffer, nil
+}
+
+// SerializeArrayNonEmptyDomainToBuffer gets and serializes the array nonempty domain and returns a Buffer object containing the payload.


Suggested change

// SerializeArrayNonEmptyDomainToBuffer gets and serializes the array nonempty domain and returns a Buffer object containing the payload.

// SerializeArrayNonEmptyDomain gets and serializes the array nonempty domain.

Fixed, thanks for catching it.

serialize.go

shaunrd0 · 2024-11-26T19:13:25Z

serialize_test.go

+	}
+	assert.NotEmpty(t, bytes.Bytes())
+	runtime.GC()
+	runtime.GC()


I don't understand the purpose of the last few lines in this test, but I see there was one like it just below so I'm probably missing something.

Should there be a check here that buffer was garbage collected, or is the test verifying something else with the explicit calls to runtime.GC()?

I copied the GC calls from the existing test, I don't know what their purpose was.

buffer_list.go

ypatia

Thanks for this very useful optimization, great job! One question, is this Core PR a prerequisite? If yes let's mark this one as Draft so that we don't accidentally merge it before Go has bundled to a Core version that includes that change.

teo-tsirpanis · 2024-11-27T09:48:58Z

Strictly speaking no, this PR can be merged without waiting for the Core PR. The Core PR is a prerequisite for fully taking advantage of the APIs added in this PR in the REST server (and even this might not be true, I have thought of a potential workaround).

It will make some stuff easier in the REST server.

NullHypothesis

Neat work, thanks for this! Left a few comments.

NullHypothesis · 2024-11-26T21:00:38Z

buffer.go

+	// Because io.Writer supports writing up to 2GB of data at a time, we have to use a loop
+	// for the bigger buffers.
+	for remaining > 0 {
+		// TODO: Use min on Go 1.21+


There's probably no harm in updating TileDB-Go's Go version. Let's address this in a separate PR.

I will bump when I do SC-58351 which needs 1.21+.

buffer.go

NullHypothesis · 2024-12-09T14:37:53Z

buffer.go

+	availableBytes := uint64(csize) - uint64(off)
+	var sizeToRead int
+	if availableBytes > math.MaxInt {
+		sizeToRead = math.MaxInt


Are we capping sizeToRead because of copy() returning an integer? If so, we may want to use io.Copy() instead because it returns an int64. In fact, I don't know if the built-in copy function even uses the io.WriterTo interface.

I'm not sure how io.Copy() is relevant here as we don't have neither a Reader nor a Writer available. See also my other comment about the choice of int and the type of slice lengths.

NullHypothesis · 2024-12-09T14:55:50Z

buffer.go

+		}
+
+		// Construct a slice from the buffer's data without copying it.
+		// Keep the buffer alive during the write, to prevent the GC from


Maybe we can use runtime.KeepAlive here, like we do in other parts of the code? Not sure if it's necessary though.

I'm not sure either if a keep alive is needed, see also my SO question. I removed it after #357 (comment), but missed the comment which I have now removed as well.

NullHypothesis · 2024-12-09T14:59:18Z

buffer_list.go

+
+	written := int64(0)
+
+	for i := uint(0); i < uint(nbuffs); i++ {


While unlikely, technically this cast could overflow, no? nbuffs is uint64 and uint(nbuffs) may be uint32.

GetBuffer() unfortunately accepts a uint.

buffer_test.go

serialize.go

teo-tsirpanis · 2024-12-10T20:08:51Z

Latest changes validated in the REST server (see CI of PR 5103, failures look unrelated).

After writing the last slice, we produced UB by incrementing cbuffer to one byte past the end of the buffer. Now we keep `cbuffer` immutable (only `remaining` gets mutated) and increment it in the statement that calls `w.Write()`. Now the test passes locally with `-d=checkptr=2`.

ypatia · 2024-12-12T12:10:57Z

We have tested this change in Core REST-CI and it is green. We have also successfully run SOMA Benchmarks using this PR and https://github.com/TileDB-Inc/TileDB-Cloud-REST/pull/5103 and have demonstrated the perf improvements.
I think it is now that testing has been completed and comments have been addressed, it is safe to get merged.

teo-tsirpanis added 8 commits October 24, 2024 04:09

Add zero-copy serialization APIs.

05a257e

Add test that serializes an array schema into a buffer.

ed67582

Deprecate Buffer.Serialize and the APIs that call it.

0ab5d35

Update a comment.

c30e838

Fix typos.

2d174df

Deprecate BufferList.Flatten.

42fe380

Remove defer inside loop.

5696a01

Use bytes.Buffer.

7812651

teo-tsirpanis requested a review from a team as a code owner November 26, 2024 17:05

Fix lint.

8da0d7f

shaunrd0 approved these changes Nov 26, 2024

View reviewed changes

Address most PR feedback.

e524c53

ypatia requested a review from a team November 27, 2024 08:47

ypatia reviewed Nov 27, 2024

View reviewed changes

teo-tsirpanis requested a review from a team November 27, 2024 09:49

teo-tsirpanis added 4 commits November 27, 2024 18:22

Use correct integer type.

4040ae7

Implement ReaderAt in Buffer.

86c8a73

It will make some stuff easier in the REST server.

Add two more APIs that were on another file.

13629dd

Move all serialization functions to serialize.go.

38155e3

teo-tsirpanis force-pushed the teo-buffer-zero-copy branch from fcfc6ff to 38155e3 Compare December 2, 2024 16:13

teo-tsirpanis requested a review from NullHypothesis December 2, 2024 16:15

NullHypothesis reviewed Dec 9, 2024

View reviewed changes

teo-tsirpanis added 3 commits December 9, 2024 17:24

Move all remaining APIs to serialization.go.

827bfe3

Fix capitalization of all error messages in serialize.go.

49d43a1

Address PR feedback and add more tests.

417820c

teo-tsirpanis force-pushed the teo-buffer-zero-copy branch from 6bf65e3 to 417820c Compare December 10, 2024 15:07

Update a comment.

f77c460

teo-tsirpanis requested a review from NullHypothesis December 10, 2024 15:20

teo-tsirpanis added 2 commits December 11, 2024 00:50

Compile tests with -d=checkptr=2.

994572e

ypatia approved these changes Dec 12, 2024

View reviewed changes

ypatia merged commit 9a66fdf into master Dec 12, 2024
10 checks passed

teo-tsirpanis deleted the teo-buffer-zero-copy branch December 12, 2024 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zero-copy serialization APIs. #357

Add zero-copy serialization APIs. #357

teo-tsirpanis commented Nov 26, 2024

shaunrd0 left a comment

shaunrd0 Nov 26, 2024

teo-tsirpanis Nov 26, 2024

shaunrd0 Nov 26, 2024

teo-tsirpanis Nov 26, 2024

shaunrd0 Nov 26, 2024

teo-tsirpanis Nov 26, 2024

ypatia left a comment

teo-tsirpanis commented Nov 27, 2024

NullHypothesis left a comment

NullHypothesis Nov 26, 2024

teo-tsirpanis Dec 9, 2024

NullHypothesis Dec 9, 2024

teo-tsirpanis Dec 9, 2024

NullHypothesis Dec 9, 2024

teo-tsirpanis Dec 10, 2024

NullHypothesis Dec 9, 2024

teo-tsirpanis Dec 10, 2024

teo-tsirpanis commented Dec 10, 2024

ypatia commented Dec 12, 2024

	// SerializeArrayNonEmptyDomainToBuffer gets and serializes the array nonempty domain and returns a Buffer object containing the payload.
	// SerializeArrayNonEmptyDomain gets and serializes the array nonempty domain.


		written := int64(0)

		for i := uint(0); i < uint(nbuffs); i++ {

Add zero-copy serialization APIs. #357

Add zero-copy serialization APIs. #357

Conversation

teo-tsirpanis commented Nov 26, 2024

shaunrd0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypatia left a comment

Choose a reason for hiding this comment

teo-tsirpanis commented Nov 27, 2024

NullHypothesis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis commented Dec 10, 2024

ypatia commented Dec 12, 2024