Stop using null terminators on JSON (de)serialization. #5353

teo-tsirpanis · 2024-10-21T11:22:54Z

This PR updates the JSON deserialization code to explicitly pass the message's length to capnproto, instead of appending a null terminator and relying on it to get the length. Correspondingly, it removes the null terminator in serialized JSON messages, which is technically a behavior breaking change but the null terminator gets already removed by both the Go API and the REST server (see story), and the serialization APIs are documented as being for internal use.

TYPE: NO_HISTORY

teo-tsirpanis · 2024-10-21T11:28:03Z

tiledb/sm/serialization/capnp_utils.h

+  // Trim null terminator if exists.
+  if (!buffer.empty() && buffer.back() == '\0') {
+    buffer = buffer.first(buffer.size() - 1);
+  }
+


On second thought this might not be needed, considering that the null terminator gets trimmed on the server side.

teo-tsirpanis · 2024-10-21T11:30:06Z

tiledb/sm/serialization/array_schema.cc

        ::capnp::MallocMessageBuilder message_builder;
        auto builder =
            message_builder.initRoot<capnp::LoadArraySchemaResponse>();
-        json.decode(kj::StringPtr(data.data()), builder);


You can validate that I did not miss a case by seeing there are zero occurences of kj::StringPtr in the codebase.

teo-tsirpanis · 2024-10-22T00:43:03Z

CI is green, apart from an unrelated failure that is expected to be fixed in #5355. This is ready for review.

…relying on the null termination.

…T server.

TileDB-Go removes it either way.

We were missing a header, and we can pass a `kj::ArrayPtr` which does not require being null-terminated.

ypatia · 2024-10-22T10:51:58Z

Question before I review this PR , what is the motivation for this change? Do you expect some performance improvement or some other sort of benefit from this refactoring?

teo-tsirpanis · 2024-10-22T11:38:35Z

@ypatia the motivation is to unify the diverging logic that depends on the format being capnp or JSON and have code always just send the whole buffer to the client. This will both simplify the code and enable some (likely significant) optimizations outlined in SC-58009.

ypatia · 2024-10-24T09:26:12Z

As discussed yesterday in planning, it'd be good to test this a bit more thoroughly just in case we are missing something that could be potentially breaking. I don't have context on why this null terminator needed to be added in the past, so I don't feel comfortable removing it just like this. Let's at least run REST-CI with JSON as default mode for this PR, @shaunrd0 said he can help with that if needed. Then we could also consider some testing using the JS client that is using JSON as serialization type, @SarantopoulosKon could help with instructions if we decide to go that way.

teo-tsirpanis · 2024-10-24T11:13:52Z

Some REST-CI tests failed with Error: Cannot serialize query; json format not supported. and then it segfaulted. Don't know yet whether it's a real problem or caused by query JSON serialization not being supported. Will try to reproduce locally.

Shelnutt2 · 2024-10-24T12:37:07Z

As discussed yesterday in planning, it'd be good to test this a bit more thoroughly just in case we are missing something that could be potentially breaking. I don't have context on why this null terminator needed to be added in the past, so I don't feel comfortable removing it just like this. Let's at least run REST-CI with JSON as default mode for this PR, @shaunrd0 said he can help with that if needed. Then we could also consider some testing using the JS client that is using JSON as serialization type, @SarantopoulosKon could help with instructions if we decide to go that way.

I believe this might indeed cause issues, as in some places it was assumed there would be a null terminator. i.e https://github.com/TileDB-Inc/TileDB-Go/blob/9ec3a9462135bf6005dcc06a1f44a6dc24afecba/buffer.go#L107

teo-tsirpanis · 2024-10-24T13:00:23Z

The attached snippet does not assume the existence of a null terminator. If there isn't one, TrimSuffix will not modify the slice.

teo-tsirpanis · 2024-10-24T17:50:13Z

I found the cause of the segfault:

TileDB/test/src/unit-capi-array.cc

Lines 1001 to 1002 in 55c5d95

    
           rc = tiledb_query_submit(ctx_, query); 
        
           CHECK(rc == TILEDB_OK);

TileDB/test/src/unit-capi-array.cc

Lines 1011 to 1018 in 55c5d95

    
           std::vector<uint64_t> fragment_timestamps; 
        
           rc = tiledb_vfs_ls( 
        
               ctx_, 
        
               vfs_, 
        
               get_commit_dir(array_path).c_str(), 
        
               &get_fragment_timestamps, 
        
               &fragment_timestamps); 
        
           CHECK(rc == TILEDB_OK);

TileDB/test/src/unit-capi-array.cc

Line 1151 in 55c5d95

rc = tiledb_array_set_open_timestamp_end(ctx_, array, fragment_timestamps[0]);

The array writes failed because serializing queries in JSON is not supported but we continued execution because of the CHECK, which means that when we ls the fragments directory we get an empty vector, which means that fragment_timestamps[0] is an out-of-bounds access. I replaced all CHECKs on query_submit in this test with REQUIREs, and that test failed gracefully on my machine. Doing this for all tests will not provide meaningful results for acceptance-testing this PR, considering how prevalent queries are in REST-CI.

@SarantopoulosKon let's sync tomorrow on how to run the Cloud-JS tests with this PR.

We can't read or write itbecause queries do not support JSON serialization.

This reverts commit c3c4fd3.

teo-tsirpanis · 2024-11-25T17:29:31Z

I added a REST CI test that creates an array using JSON serialization. Let me know if this would be sufficient for validating this PR.

ypatia

Thanks for this change and for you effort to test it. If all goes well, it will enable an important optimization server side that we believe it can bring both memory and latency improvements.

teo-tsirpanis · 2024-11-26T11:45:00Z

CI is green, merging…

[SC-58051](https://app.shortcut.com/tiledb-inc/story/58051/stop-using-null-terminators-on-json-de-serialization) This PR updates the JSON deserialization code to explicitly pass the message's length to capnproto, instead of appending a null terminator and relying on it to get the length. Correspondingly, it removes the null terminator in serialized JSON messages, which is technically a behavior breaking change but the null terminator gets already removed by both [the Go API](https://github.com/TileDB-Inc/TileDB-Go/blob/9ec3a9462135bf6005dcc06a1f44a6dc24afecba/buffer.go#L105-L107) and the REST server (see story), and the serialization APIs are documented as being for internal use. --- TYPE: NO_HISTORY

teo-tsirpanis commented Oct 21, 2024

View reviewed changes

teo-tsirpanis marked this pull request as ready for review October 22, 2024 00:43

teo-tsirpanis requested review from ypatia and shaunrd0 October 22, 2024 00:43

teo-tsirpanis added 5 commits October 22, 2024 10:32

Explicitly pass the string's length on JSON deserialization and stop …

ee09130

…relying on the null termination.

Stop adding null terminators on JSON responses retrieved from the RES…

b9f3330

…T server.

Stop adding null terminator to serialized JSON messages.

c0ba232

TileDB-Go removes it either way.

Fix errors.

ffd64fd

We were missing a header, and we can pass a `kj::ArrayPtr` which does not require being null-terminated.

Fix test.

98adaf4

teo-tsirpanis force-pushed the teo/json-null-term branch from 1878cb1 to 98adaf4 Compare October 22, 2024 07:32

Set REST server serialization format to JSON.

c3c4fd3

teo-tsirpanis added 3 commits November 14, 2024 16:01

Merge branch 'dev' into teo/json-null-term

06f14a4

Add REST-CI test to create a remote array with JSON serialization.

73714fa

We can't read or write itbecause queries do not support JSON serialization.

Revert "Set REST server serialization format to JSON."

11b0beb

This reverts commit c3c4fd3.

ypatia approved these changes Nov 26, 2024

View reviewed changes

Merge branch 'dev' into teo/json-null-term

d017ba0

teo-tsirpanis merged commit 414a261 into dev Nov 26, 2024
63 checks passed

teo-tsirpanis deleted the teo/json-null-term branch November 26, 2024 11:45

ypatia mentioned this pull request Nov 27, 2024

Add zero-copy serialization APIs. TileDB-Inc/TileDB-Go#357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop using null terminators on JSON (de)serialization. #5353

Stop using null terminators on JSON (de)serialization. #5353

teo-tsirpanis commented Oct 21, 2024

teo-tsirpanis Oct 21, 2024

teo-tsirpanis Oct 21, 2024

teo-tsirpanis commented Oct 22, 2024

ypatia commented Oct 22, 2024

teo-tsirpanis commented Oct 22, 2024

ypatia commented Oct 24, 2024

teo-tsirpanis commented Oct 24, 2024 •

edited

Loading

Shelnutt2 commented Oct 24, 2024

teo-tsirpanis commented Oct 24, 2024

teo-tsirpanis commented Oct 24, 2024

teo-tsirpanis commented Nov 25, 2024

ypatia left a comment

teo-tsirpanis commented Nov 26, 2024

Stop using null terminators on JSON (de)serialization. #5353

Stop using null terminators on JSON (de)serialization. #5353

Conversation

teo-tsirpanis commented Oct 21, 2024

teo-tsirpanis Oct 21, 2024

Choose a reason for hiding this comment

teo-tsirpanis Oct 21, 2024

Choose a reason for hiding this comment

teo-tsirpanis commented Oct 22, 2024

ypatia commented Oct 22, 2024

teo-tsirpanis commented Oct 22, 2024

ypatia commented Oct 24, 2024

teo-tsirpanis commented Oct 24, 2024 • edited Loading

Shelnutt2 commented Oct 24, 2024

teo-tsirpanis commented Oct 24, 2024

teo-tsirpanis commented Oct 24, 2024

teo-tsirpanis commented Nov 25, 2024

ypatia left a comment

Choose a reason for hiding this comment

teo-tsirpanis commented Nov 26, 2024

teo-tsirpanis commented Oct 24, 2024 •

edited

Loading